Update nom dependency in Cargo.toml #47

rekanorman · 2020-01-16T23:49:55Z

This change gives users the option of using nom_locate without needing to use the default features of nom, e.g. "lexical".

This change is necessary to allow nom_locate to be used without using the default features of nom, e.g. "lexical".

rekanorman · 2020-01-16T23:53:42Z

@fflorent would you mind merging this change so that we can use nom_locate without needing to include all the default dependencies of nom, such as "lexical-core".

progval · 2020-01-16T23:54:27Z

@rekanorman I'll review this later in the week

rekanorman · 2020-01-16T23:55:00Z

@progval thanks!

rekanorman · 2020-01-21T21:42:40Z

@progval just checking if there's any update on this?

rekanorman · 2020-02-03T11:06:04Z

Hi @progval, @fflorent, just checking in again to see if there's been any update.

progval · 2020-02-03T12:19:49Z

Ugh sorry, I still haven't found the time to look at this

progval · 2020-02-03T17:25:45Z

Okay, that looks good to me. Could you just fix the tests so they run on Travis? This would mostly involve skipping some tests (eg. #[cfg(not(feature="std"))])

I don't think it's possible to make FEATURES="allow" work on stable or beta, so you should tell Travis that they are allowed to fail

Since a lot of stuff in nom_locate depends on nom having the "std" feature, it probably makes sense to require this feature. Other nom features such as "regexp" and "lexical" will still be optional.

…ure.

…loc feature." This reverts commit 6b72f8b.

rekanorman · 2020-02-04T03:52:01Z

Thanks for taking a look.

It looks like the line #![cfg_attr(all(not(feature = "std"), feature = "alloc"), feature(alloc))] in nom might be an error. There's currently no compiler feature called alloc, possibly this was an old feature which no longer exists?

Instead of allowing the tests to fail on stable and beta, I thought it might be nicer to just require the "std" feature in the nom dependency. The "lexical" feature will still be optional.

Let me know what you think. If you'd prefer to allow the tests to fail instead, I'm happy to make this change.

progval · 2020-02-04T07:46:10Z

It looks like the line #![cfg_attr(all(not(feature = "std"), feature = "alloc"), feature(alloc))] in nom might be an error. There's currently no compiler feature called alloc, possibly this was an old feature which no longer exists?

hmm, last I checked, the alloc compiler feature was used to determine if the alloc crate was available. It looks like it's always available now? Weird.

I'll look into it later.

Instead of allowing the tests to fail on stable and beta, I thought it might be nicer to just require the "std" feature in the nom dependency.

No, nom_locate shouldn't unconditionally depend on nom's std. If any dependency requires std, it means the whole crate depends on std, which defeats the purpose

…loc" on stable/beta.

rekanorman · 2020-02-04T09:55:36Z

No, nom_locate shouldn't unconditionally depend on nom's std. If any dependency requires std, it means the whole crate depends on std, which defeats the purpose

That makes sense.

Sorry, I was confused before and didn't understand that alloc was a nightly-only feature. I belived I've made the correct change now, let me know what you think.

progval · 2020-02-04T11:08:34Z

.travis.yml

-    - rust: stable
-      env: FEATURES='alloc'
    - rust: stable
      env: FEATURES=''

    - rust: beta
      env: FEATURES='default'
-    - rust: beta
-      env: FEATURES='alloc'


don't remove these; the jobs should be in both (yeah I know, it doesn't make sense, but that's how Travis works)

progval

Sorry, one last thing I should have caught earlier

progval · 2020-02-04T23:04:53Z

tests/integration_tests.rs

@@ -10,6 +10,7 @@ use std::ops::{Range, RangeFull};
 type StrSpan<'a> = LocatedSpan<&'a str>;
 type BytesSpan<'a> = LocatedSpan<&'a [u8]>;

+#[cfg(feature = "alloc")]


Should be:

#[cfg(any(feature = "std", feature = "alloc"))]

because std does not imply alloc.

(same for the other occurences below)

…e = "alloc"))]

progval · 2020-02-05T07:55:54Z

Thanks!

* Update nom dependency in Cargo.toml This change is necessary to allow nom_locate to be used without using the default features of nom, e.g. "lexical". * Change nom dependency to require "std" feature. Since a lot of stuff in nom_locate depends on nom having the "std" feature, it probably makes sense to require this feature. Other nom features such as "regexp" and "lexical" will still be optional. * Add #[cfg(feature = alloc)] to tests which require the nom alloc feature. * Revert "Add #[cfg(feature = alloc)] to tests which require the nom alloc feature." This reverts commit 6b72f8b. * Remove "std" feature from nom dependency, and allow failures with "alloc" on stable/beta. * Add removed jobs back to travis.yml include section. * Add missing quote * Change #[cfg(feature = "alloc")] to #[cfg(any(feature = "std", feature = "alloc"))]

rekanorman · 2020-02-05T09:14:29Z

Thanks for all your help!
Would you mind publishing a version with these changes to crates.io?

Update nom dependency in Cargo.toml

9470b2a

This change is necessary to allow nom_locate to be used without using the default features of nom, e.g. "lexical".

progval self-requested a review January 16, 2020 23:53

rekanorman and others added 3 commits February 4, 2020 10:39

Change nom dependency to require "std" feature.

8167cf4

Since a lot of stuff in nom_locate depends on nom having the "std" feature, it probably makes sense to require this feature. Other nom features such as "regexp" and "lexical" will still be optional.

Add #[cfg(feature = alloc)] to tests which require the nom alloc feat…

6b72f8b

…ure.

Revert "Add #[cfg(feature = alloc)] to tests which require the nom al…

0680e0a

…loc feature." This reverts commit 6b72f8b.

Remove "std" feature from nom dependency, and allow failures with "al…

2037f3b

…loc" on stable/beta.

progval reviewed Feb 4, 2020

View reviewed changes

rekanorman added 2 commits February 5, 2020 08:49

Add removed jobs back to travis.yml include section.

f0d7068

Add missing quote

ba4b66a

progval requested changes Feb 4, 2020

View reviewed changes

Change #[cfg(feature = "alloc")] to #[cfg(any(feature = "std", featur…

a0ff73f

…e = "alloc"))]

progval merged commit aa72618 into fflorent:master Feb 5, 2020

rekanorman deleted the change-dependency branch February 5, 2020 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nom dependency in Cargo.toml #47

Update nom dependency in Cargo.toml #47

rekanorman commented Jan 16, 2020

rekanorman commented Jan 16, 2020

progval commented Jan 16, 2020

rekanorman commented Jan 16, 2020

rekanorman commented Jan 21, 2020

rekanorman commented Feb 3, 2020

progval commented Feb 3, 2020

progval commented Feb 3, 2020

rekanorman commented Feb 4, 2020

progval commented Feb 4, 2020 •

edited

Loading

rekanorman commented Feb 4, 2020

progval Feb 4, 2020

rekanorman Feb 4, 2020

progval left a comment

progval Feb 4, 2020

rekanorman Feb 4, 2020

progval commented Feb 5, 2020

rekanorman commented Feb 5, 2020

Update nom dependency in Cargo.toml #47

Update nom dependency in Cargo.toml #47

Conversation

rekanorman commented Jan 16, 2020

rekanorman commented Jan 16, 2020

progval commented Jan 16, 2020

rekanorman commented Jan 16, 2020

rekanorman commented Jan 21, 2020

rekanorman commented Feb 3, 2020

progval commented Feb 3, 2020

progval commented Feb 3, 2020

rekanorman commented Feb 4, 2020

progval commented Feb 4, 2020 • edited Loading

rekanorman commented Feb 4, 2020

progval Feb 4, 2020

Choose a reason for hiding this comment

rekanorman Feb 4, 2020

Choose a reason for hiding this comment

progval left a comment

Choose a reason for hiding this comment

progval Feb 4, 2020

Choose a reason for hiding this comment

rekanorman Feb 4, 2020

Choose a reason for hiding this comment

progval commented Feb 5, 2020

rekanorman commented Feb 5, 2020

progval commented Feb 4, 2020 •

edited

Loading