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

Add configuration entry for cargo's rustc command. #1079

Merged
merged 1 commit into from Sep 10, 2016

Conversation

Projects
None yet
5 participants
@c-nixon
Contributor

c-nixon commented Sep 9, 2016

This new configuration entry allows arguments to be passed to cargo's
rustc subcommand, mirroring the preexisting configuration entry
for rustc itself.

This enables the user to specify cargo related flags like --features,
number of worker threads with much be passed to cargo rust before the --
which delineates the rustc subcommand's args from rustc proper's

@CLAassistant

This comment has been minimized.

CLAassistant commented Sep 9, 2016

CLA assistant check
All committers have signed the CLA.

flycheck.el Outdated
@@ -8380,6 +8380,9 @@ See URL `http://jruby.org/'."
:modes (enh-ruby-mode ruby-mode)
:next-checkers ((warning . ruby-rubylint)))
(flycheck-def-args-var flycheck-cargo-rustc-args (rust-cargo)
:package-version '(flycheck . "0.24"))

This comment has been minimized.

@fmdkdd

fmdkdd Sep 10, 2016

Member

This is supposed to indicate the flycheck version number that introduced this variable. In this case, this should be "30" rather than "0.24".

@c-nixon c-nixon force-pushed the c-nixon:master branch from 4c345ed to 907d61f Sep 10, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 10, 2016

Thank you for your contribution.

Out of curiosity, how do you pass this variable to flycheck in your use case? A dir-locals file? A file-local variable? In your dotfile for every rust project? Can you give an example? Keep in mind I have never used the flycheck-rust-args variable, so I'm asking in order to have a better picture of our users's needs.

In any case, I'm in favor to merge. Before that however, could you please make the following changes:

  1. Add a line documenting this variable in doc/languages.rst. Look for the documentation of flycheck-rust-args. I think this is what blocks the CI; you can run the tests locally before pushing, see the Contributor's guide.
  2. Change the package-version argument as per my inline comment
  3. Add a line in CHANGES.rst under the New features heading to signal the new variable
flycheck.el Outdated
@@ -8380,6 +8380,9 @@ See URL `http://jruby.org/'."
:modes (enh-ruby-mode ruby-mode)
:next-checkers ((warning . ruby-rubylint)))
(flycheck-def-args-var flycheck-cargo-rustc-args (rust-cargo)
:package-version '(flycheck . "0.30"))

This comment has been minimized.

@fmdkdd

fmdkdd Sep 10, 2016

Member

That was quick! But it's just 30 without the 0.. We have dropped the decimal since version 26.

@c-nixon

This comment has been minimized.

Contributor

c-nixon commented Sep 10, 2016

Thanks for the quick review!

I use a combination currently. By default I have dir-locals files with --features clippy and flycheck-rust-args with -Z extra-plugins=clippy in my own project directories (to avoid polluting my .rs files with #![feature(clippy)]) and have jobs limited to 2 via customize on my laptop because it's dieing and doesn't like having all 4 cores pegged for any length of time, especially on battery.

I'll fix up the numbering, add the docs and changelog once i'm back at a machine with a keyboard!

@c-nixon c-nixon force-pushed the c-nixon:master branch 2 times, most recently from f7e384a to 801dbc5 Sep 10, 2016

@c-nixon

This comment has been minimized.

Contributor

c-nixon commented Sep 10, 2016

I believe that's everything addressed

@c-nixon c-nixon force-pushed the c-nixon:master branch from 801dbc5 to 6c5ab4f Sep 10, 2016

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 10, 2016

Great! Good call on enhancing the documentation to flycheck-rust-args as well.

LGTM.

I'll leave the rest to the @flycheck/maintainers.

CHANGES.rst Outdated
@@ -8,6 +8,11 @@
own ``.luacheckrc`` detection. Therefore ``flycheck-luacheckrc`` is
no longer used [GH-1057]
- New features:

This comment has been minimized.

@lunaryorn

lunaryorn Sep 10, 2016

Contributor

Thanks for updating the changelog, @c-nixon. However, please move the "New features" item below the "new syntax checkers" item. We think that new syntax checkers are more important as the extend the languages Flycheck supports.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 10, 2016

@c-nixon I'm sorry if I'm asking a stupid question but why don't you use flycheck-rust-args? As far as I can see it's already used in rust and rust-cargo

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 10, 2016

@lunaryorn The cargo rustc command accepts two different sets of options, one set is intended for cargo, and the other is intended for rustc. The second set must appear after the -- separator:

cargo rustc [options] [--] [<opts>...]

So this PR adds the ability to extend the first set of option, which is specific to the rust-cargo checker, while the existing flycheck-rust-args variable can only extend the second set, but can do so in both rust and rust-cargo.

Add configuration entry for cargo's rustc command.
This new configuration entry allows arguments to be passed to cargo's
rustc subcommand, mirroring the preexisting configuration entry
for rustc itself.

This enables the user to specify cargo related flags like --features,
number of worker threads with much be passed to cargo rust before the --
which delineates the rustc subcommand's args from rustc proper's

@c-nixon c-nixon force-pushed the c-nixon:master branch from 6c5ab4f to 82b3be1 Sep 10, 2016

@c-nixon

This comment has been minimized.

Contributor

c-nixon commented Sep 10, 2016

That's the changelog fixed, and @fmdkdd explained the reasoning far better than I would have :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 10, 2016

Oh dear, that was a really stupid question of mine. Thanks for enlightening me 😊

LGTM.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 10, 2016

@fmdkdd While we're waiting for @cpitclaudel's review I'd like to thank you for the thorough review of this pull request. Many thanks 👍

Would you like to join us as a maintainer, to be able to formally approve pull requests?

@c-nixon Thanks to you, of course, for your contribution, and for going through the review!

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 10, 2016

LGTM; thanks @c-nixon for this contribution and @fmdkdd for your review!

@lunaryorn lunaryorn merged commit fc96ef8 into flycheck:master Sep 10, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 10, 2016

@c-nixon Well done! 👏

@lunaryorn I'm humbled by the offer, but for the moment I'm afraid I cannot commit to review every PR. There's a lot that happens here in any given week! I tend to stick to rust-related issues because I'm not yet comfortable with the rest of flycheck. But I'll keep on helping in any capacity I can.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 14, 2016

@fmdkdd You're welcome, thank you very much for your contributions and your support! 😍 Taking care of Rust-related issues already helps us a lot 👍 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment