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

Custom attributes parser #198

Merged
merged 19 commits into from
Jun 24, 2019
Merged

Conversation

sphynx
Copy link
Contributor

@sphynx sphynx commented Jun 8, 2019

Closes #178

I've added a new module: parse.rs, which contains an intermediate representation for structopt attributes. This representation reflects domain-specific attributes of structopt and is used instead of generic syn.Meta, which played a similar role in the previous version of the code. This allows parsing arbitrary expressions instead of String literals in attribute values. I tried to keep those intermediate structures low-level, i.e. closer to the token level, so that it's easy to write Parse trait implementations for them. Later, they are converted to more high-level representation in attrs module.

I didn't change the parsing of doc-comments, since they are not affected by the introduction of arbitrary tokens in attributes in Rust 1.34. So syn.Meta is still used there.

Please take a look at the code: if you like the approach, I'll add some notes to the documentation and the ChangeLog.

Thank you!

@sphynx
Copy link
Contributor Author

sphynx commented Jun 8, 2019

My newly added tests obviously can't be compiled on Rust 1.21.0, because the feature in question is supported in 1.34.0. I'm not sure how to proceed with those tests in the best way.

The issue is similar to arg_enum! from clap which is present in examples, but it's a test, not an example. I'm not sure how to disable a single test module (can't find anything useful in cargo test --help).

Perhaps I can [ignore] those tests by default and turn them on in all the cases except Rust 1.21.0? It's not ideal though. Or I can try to introduce a custom build attribute and use it with cfg?

@TeXitoi
Copy link
Owner

TeXitoi commented Jun 8, 2019

We can just skip the tests on rust 1.21. If it compils, and tests are passing using latest stable rust, there is no reason that it would not work with 1.21 for the usable features.

@TeXitoi
Copy link
Owner

TeXitoi commented Jun 8, 2019

I may take a bit of time to review this PR as it's quite long.

@sphynx
Copy link
Contributor Author

sphynx commented Jun 8, 2019

We can just skip the tests on rust 1.21. If it compils, and tests are passing using latest stable rust, there is no reason that it would not work with 1.21 for the usable features.

Ok, so we should update .travis.yml for that, right? I'm not sure how to do it, my guess is that overriding script for 1.21.0 can work:

    - rust: 1.21.0
      # just build the code for 1.21.0, since some of the tests use features
      # which are unsupported in 1.21.0
      script: cargo build

Is it the right way to do it in Travis? If so, I can add that change to this PR.

@sphynx
Copy link
Contributor Author

sphynx commented Jun 8, 2019

Is it the right way to do it in Travis? If so, I can add that change to this PR.

Ok, I've changed .travis.yaml, we'll see how it goes :)

@TeXitoi
Copy link
Owner

TeXitoi commented Jun 8, 2019

Yep, it's working.

@sphynx
Copy link
Contributor Author

sphynx commented Jun 9, 2019

Note that 'full' version of syn is needed for this change to support parsing of more complicated expressions like &[AppSettings::Foo, AppSetting::Bar].

A quote from syn documentation:

full — Data structures for representing the syntax tree of all valid Rust source code, including items and expressions.

@TeXitoi
Copy link
Owner

TeXitoi commented Jun 10, 2019

As noted in #200, v0.2.17 was a breaking change version. Thus, I've yanked the version, and updated the minimal rust version to 1.34. The plan is to put the new version in v0.3. v0.3 "need" a edition 2018 before publishing.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! It will be a big improvement!

structopt-derive/src/parse.rs Show resolved Hide resolved
tests/custom-string-parsers.rs Outdated Show resolved Hide resolved
tests/non_literal_attributes.rs Outdated Show resolved Hide resolved
@sphynx
Copy link
Contributor Author

sphynx commented Jun 18, 2019

I've implemented the requested changes, could you please take a look again?

Thank you!

@sphynx
Copy link
Contributor Author

sphynx commented Jun 20, 2019

I've improved the error messages from parse.rs, deleted obsoleted trybuild test cases and rebased several others to contain updated error messages.

As a side note: this pull request is getting longer and longer (already at 18 commits!) and it's becoming harder to maintain: to rebase it or to merge the other changes in...

@TeXitoi
Copy link
Owner

TeXitoi commented Jun 20, 2019

Yes, sorry, that's the priority now. Will try to review it soon. No other PR will be merged before this one.

@TeXitoi TeXitoi mentioned this pull request Jun 24, 2019
Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great!

Just a small test to remove, and I merge this PR.

Thanks again!

tests/arguments.rs Outdated Show resolved Hide resolved
@TeXitoi TeXitoi merged commit f94b26c into TeXitoi:master Jun 24, 2019
chrissimpkins added a commit to chrissimpkins/cargo-bisect-rustc that referenced this pull request Feb 26, 2020
chrissimpkins added a commit to chrissimpkins/cargo-bisect-rustc that referenced this pull request Feb 26, 2020
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.

Attribute token streams for raw calls (Rust 1.34)
2 participants