Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-6835: [Archery][CMake] Restore ARROW_LINT_ONLY cmake option #5616

Conversation

fsaintjacques
Copy link
Contributor

No description provided.

@fsaintjacques fsaintjacques changed the title ARROW-6832: [Archery][CMake] Restore ARROW_LINT_ONLY cmake option ARROW-6835: [Archery][CMake] Restore ARROW_LINT_ONLY cmake option Oct 10, 2019
@fsaintjacques fsaintjacques force-pushed the ARROW-6835-restore-arrow-lint-only branch from ad2ce3f to 0f77156 Compare October 10, 2019 12:05
@fsaintjacques
Copy link
Contributor Author

@pitrou can you try locally to see if it restores your local script and also if archery works without conflict over CMAKE env var.

@pitrou
Copy link
Member

pitrou commented Oct 10, 2019

ARROW_ONLY_LINT still doesn't work. I get the same double-conversion errors in CMake.

@pitrou
Copy link
Member

pitrou commented Oct 10, 2019

As for archery lint, it now works, thank you :-)

I would have two suggestions though:

  1. add domain-only options, e.g. I'd like to run archery lint --only-cpp --fix, and it would run and fix all C++ issues (but only them). Currently I have to disable all other linters by hand (they take some time and they may not be available, e.g. I don't have "hadolint" on my machine).
  2. boolean options should not require an explicit boolean. It's weird to write --fix 1 instead of simply --fix.

@github-actions
Copy link

@pitrou
Copy link
Member

pitrou commented Oct 10, 2019

So, ARROW_ONLY_LINT isn't very important for me if archery lint works with minimal fuss and minimal runtime as currently.

@fsaintjacques fsaintjacques force-pushed the ARROW-6835-restore-arrow-lint-only branch from 5c87af9 to 1f0d965 Compare October 10, 2019 13:01
@fsaintjacques
Copy link
Contributor Author

As for archery lint, it now works, thank you :-)

I would have two suggestions though:

  1. add domain-only options, e.g. I'd like to run archery lint --only-cpp --fix, and it would run and fix all C++ issues (but only them). Currently I have to disable all other linters by hand (they take some time and they may not be available, e.g. I don't have "hadolint" on my machine).

I just added a commit that will downgrade gracefully if a required binary (cmake, hadolint, flake8, cargo) is missing. It'll output a warning message and skip the linter. This should provide a better first experience than having to toggle options (thanks for being my guinea pig).

In the next iteration, I want to bring the detect-changes in archery so it can dynamically decide which linter to run by default invocation of archery lint. A second option I want to explore is a configuration file that toggles archery's default for some sub-commands, e.g. cmake options, linter options, etc.

  1. boolean options should not require an explicit boolean. It's weird to write --fix 1 instead of simply --fix.

In my first draft, all boolean options were flags à-la autoconf (--with-clang-format/--without-clang-format). This is very convenient for humans, not so much for scripts. I decided to go with the --with-clang-format=X to ease porting CI scripts since we can do something like

archery lint --with-clang-format=${ARROW_CI_CPP_ENABLE} ...

This is much simpler than building argv via append and conditions (at least in bash).

@fsaintjacques fsaintjacques force-pushed the ARROW-6835-restore-arrow-lint-only branch from 1f0d965 to 2b6c46b Compare October 10, 2019 13:14
@wesm wesm force-pushed the ARROW-6835-restore-arrow-lint-only branch from 2b6c46b to 02e6605 Compare October 11, 2019 14:48
Make --fix in archery lint a flag [skip ci]
@wesm wesm force-pushed the ARROW-6835-restore-arrow-lint-only branch from 02e6605 to 55bcb89 Compare October 11, 2019 14:49
@wesm
Copy link
Member

wesm commented Oct 11, 2019

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants