Page MenuHomePhabricator
Differential D4568

[Chore] Lint native/android/app/CMakeLists.txt
ClosedPublic
Actions

Authored by • jon on Jul 18 2022, 4:23 PM.
Tags
None
Referenced Files
F3280284: D4568.id14603.diff
Sat, Nov 16, 9:06 AM
F3280147: D4568.id14722.diff
Sat, Nov 16, 9:03 AM
Unknown Object (File)
Thu, Oct 31, 4:23 PM
Unknown Object (File)
Sun, Oct 27, 6:55 AM
Unknown Object (File)
Sun, Oct 27, 6:55 AM
Unknown Object (File)
Sun, Oct 27, 6:55 AM
Unknown Object (File)
Sun, Oct 27, 6:55 AM
Unknown Object (File)
Sun, Oct 27, 6:50 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
tomek

Details

Reviewers
atul
varun
• max
• karol
Commits
rCOMM008e4c69e77b: [Chore] Lint native/android/app/CMakeLists.txt
Summary

Lint android's CMakeLists.txt

Depends on D4564

Test Plan
nix develop
cmake-lint native/android/app/CMakeLists.txt
./gradlew bundleRelease --no-daemon # ensure build still passes

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jon created this revision.Jul 18 2022, 4:23 PM
Herald added subscribers: • abosh, • adrian, tomek, ashoat. · View Herald TranscriptJul 18 2022, 4:23 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 18 2022, 4:28 PM
Harbormaster failed remote builds in B10605: Diff 14598!
• jon updated this revision to Diff 14603.Jul 18 2022, 4:42 PM

Rebase

• jon added a child revision: D4548: [BuildKite] Add cmake lint gate.Jul 18 2022, 4:43 PM
Harbormaster completed remote builds in B10610: Diff 14603.Jul 18 2022, 4:52 PM
• jon requested review of this revision.Jul 18 2022, 4:52 PM
ashoat added a comment.Jul 19 2022, 8:19 AM

Defining the dirs separately makes a lot of sense – I assume you did that manually @jonringer-comm, but will be very impressed with the Linter if it does that automatically!

Separately, noticed that some of the lines here are longer than 80 chars, but for filenames I think it makes sense... it definitely hurts readability to split those across lines. Ideally we can have something that works like Prettier's printWidth setting, which generally limits line length but excludes import lines.

atul added a comment.Jul 19 2022, 10:59 AM

noticed that some of the lines here are longer than 80 chars, but for filenames I think it makes sense...

Strongly agree we shouldn't try splitting filenames in weird ways to stay within 80 char limit

atul accepted this revision.Jul 19 2022, 11:01 AM

Looks good

This revision is now accepted and ready to land.Jul 19 2022, 11:01 AM
• jon added a comment.Jul 20 2022, 11:44 AM

I assume you did that manually @jonringer-comm

I did it manually. The cmake-format command will mostly do whitespace and paren placement.

Closed by commit rCOMM008e4c69e77b: [Chore] Lint native/android/app/CMakeLists.txt (authored by • jon). · Explain WhyJul 20 2022, 12:00 PM
This revision was automatically updated to reflect the committed changes.
• jon added a commit: rCOMM008e4c69e77b: [Chore] Lint native/android/app/CMakeLists.txt.

Revision Contents
Changeset List

PathSize
native/
android/
app/
142 lines

Diff 14598

View Options

native/android/app/CMakeLists.txt

Loading...