Add support for 'required-features'. by little-arhat · Pull Request #69 · flycheck/flycheck-rust

little-arhat · 2018-08-23T22:31:48Z

Discover 'required-features' in Cargo manifest and initialize value of 'rust-cargo-features'. Fixes #68.

fmdkdd

Good call on making an alist. It's probably more readable that way with multiple features.

Do you think you could add a test that gets the required-features from a cargo project?

fmdkdd · 2018-08-24T09:26:14Z

flycheck-rust.el

-kind (lib, bin, test, example or bench), and NAME the target
-name (usually, the crate name).  If FILE-NAME exactly matches a
-target `src-path', this target is returned.  Otherwise, return
+Return a alist ((KIND . k) (NAME . n) (REQUIRED-FEATURES . rf))


Return an alist

fmdkdd · 2018-08-24T09:26:27Z

flycheck-rust.el

+Return a alist ((KIND . k) (NAME . n) (REQUIRED-FEATURES . rf))
+where KIND is the target kind (lib, bin, test, example or bench),
+NAME the target name (usually, the crate name), and REQUIRED-FEATURES is the
+optional list of features required to build selected target.  If FILE-NAME


to build the selected target

tests/test-rust-setup.el

little-arhat · 2018-08-24T19:53:31Z

@fmdkdd yeah, sure! Didn't want to add more code for tests without knowing it looks more or less ok %)

Thanks for comments, fixed.

little-arhat · 2018-08-24T20:47:00Z

Hm, tests are failing since cargo there doesn't have "required-features" in cargo metadata output. Should I modify test suite to compare against either alist with required-features or the old one?

fmdkdd · 2018-08-27T10:46:53Z

Shouldn't the nightly cargo already emit required-features? Looks like the cargo version is trailing behind:

cargo 1.29.0-nightly (6a7672ef5 2018-08-14)

The test will break for beta and stable, but should eventually get there. In the meantime, we should skip it for older versions. We can (signal 'buttercup-pending "requires cargo 1.29") to mark the test as pending. We could do that after inspecting cargo --version.

little-arhat · 2018-08-27T22:07:07Z

@fmdkdd yeah, it's lagging behind rustc, not sure what the actual release schedule cargo has. Anyways, it should hit nightly channel soon and then tests will pass for nightly. For other channels I've added check that disables tests for older cargos.

Thanks!

fmdkdd

Thanks. I have some stylistic changes, and one question about the actual version number. See the review comments.

fmdkdd · 2018-08-28T12:53:44Z

tests/test-rust-setup.el

+      :to-equal '((kind . "lib") (name . "lib-test"))))
+
+ (it "'src/main.rs' to the bin target with required-features (fea1)"
+     (if (cargo-older-than-1.29-nightly)


Better to use when instead of an empty then branch in the if.

True, will fix!

fmdkdd · 2018-08-28T12:56:09Z

tests/test-rust-setup.el

+
+(defun get-cargo-version ()
+  (let ((cargo (funcall flycheck-executable-find "cargo")))
+    (with-temp-buffer


with-output-to-string would be shorter here.

I was looking for smth like that, cool, thanks.

fmdkdd · 2018-08-28T13:56:15Z

tests/test-rust-setup.el

+
+(defun cargo-older-than-1.29-nightly ()
+  (let* ((version-parts (cargo-version))
+         (version (car version-parts))


Can be shortened with:

(pcase-let ((`(,version ,channel) (cargo-version))) ...)

fmdkdd · 2018-08-28T14:00:16Z

tests/test-rust-setup.el

+  (let* ((version-parts (cargo-version))
+         (version (car version-parts))
+         (channel (cadr version-parts)))
+    (if (version< version "1.29")


More idiomatic would be:

(or (version< version "1.29") (and (version= version "1.29") (string= channel "beta")))

But wait, the name of the function implies we return t when cargo --version is more than 1.29-nightly, but we actually do the reverse. This should be fixed.

Function name is cargo-version-is-older-than-1.29, so for 1.28.0 it will return t (1.28.0 came out before 1.29.0, so it's older). Perhaps "older" is a bit confusing?

Argh, excuse the brainfart. older is not confusing at all, I was just confused.

fmdkdd · 2018-08-28T14:02:09Z

tests/test-rust-setup.el

+         (channel (cadr version-parts)))
+    (if (version< version "1.29")
+        t
+      (and (version= version "1.29") (string= channel "beta")))))


Do we actually know which version required-features will be included in? If it's 1.30, then we don't need to check for the channel, right?

Current beta has cargo 1.29.0-beta (<sha> 2018-08-05) (or some such). Nighty should get this feature in a week at most, and the version will be 1.29.0-nightly (<sha> 2018-08-21). We can just test for 1.30, but then we will disable test for nightly 1.29 even though cargo will have new feature.

Not a big deal, I guess, and will help make code a bit nicer.

Ah sure, let's test with 1.29-nightly. No big deal.

Discover 'required-features' in Cargo manifest and initialize value of 'rust-cargo-features'.

little-arhat · 2018-09-04T08:42:13Z

Apparently, cargo is not bumped automatically for nightly channel, but PR has been posted, so it should be available in next nighly!

@fmdkdd do you have any other comments I should address? %)

fmdkdd · 2018-09-04T11:18:00Z

Apparently, cargo is not bumped automatically for nightly channel,

Interesting! Thanks for keeping us posted.

do you have any other comments I should address?

Nope, LGTM. Good job and thank you for the contribution. 👏

little-arhat mentioned this pull request Aug 23, 2018

Get "required-features" for target and set them #68

Closed

fmdkdd requested changes Aug 24, 2018

View reviewed changes

little-arhat force-pushed the feature-required-features-support branch from 6538525 to 24e0edd Compare August 24, 2018 19:52

little-arhat force-pushed the feature-required-features-support branch from 24e0edd to bb78041 Compare August 27, 2018 22:05

little-arhat force-pushed the feature-required-features-support branch 4 times, most recently from 4ba335f to 934bcb2 Compare August 28, 2018 08:41

fmdkdd requested changes Aug 28, 2018

View reviewed changes

Add support for 'required-features'.

7e491e9

Discover 'required-features' in Cargo manifest and initialize value of 'rust-cargo-features'.

little-arhat force-pushed the feature-required-features-support branch from 934bcb2 to 7e491e9 Compare August 28, 2018 19:01

fmdkdd approved these changes Sep 4, 2018

View reviewed changes

fmdkdd merged commit f1220cc into flycheck:master Sep 4, 2018

Uh oh!

Comments

Conversation

little-arhat commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmdkdd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

little-arhat commented Aug 24, 2018

Uh oh!

little-arhat commented Aug 24, 2018

Uh oh!

fmdkdd commented Aug 27, 2018

Uh oh!

little-arhat commented Aug 27, 2018

Uh oh!

fmdkdd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

little-arhat commented Sep 4, 2018

Uh oh!

fmdkdd commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

little-arhat commented Aug 23, 2018 •

edited

Loading