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

Give ParsedAttribute::Property a Token rather than a Literal? #24

Open
mkj opened this issue May 31, 2022 · 3 comments
Open

Give ParsedAttribute::Property a Token rather than a Literal? #24

mkj opened this issue May 31, 2022 · 3 comments
Labels
enhancement New feature or request not-stale

Comments

@mkj
Copy link
Contributor

mkj commented May 31, 2022

Thanks for virtue, it works nicely for my purposes parsing SSH binary protocol.
I'm using enum variant attributes that can have Idents in them such as #[sshwire(variant = SSH_NAME_ED25519)], where the SSH_NAME_ED25519 is a const &'static str. That means ParsedAttribute won't work since it only allows a Literal value.

Would it make sense to allow any TokenTree for Property, rather than just a Literal? (I guess Punct wouldn't make sense).

@VictorKoenders
Copy link
Contributor

I'm up for changing ParsedAttribute ::Property. Some options I see:

  • Make ParsedAttribute::Property(Ident, TokenTree);
  • Make ParsedAttribute::LiteralProperty(Ident, Literal); and ParsedAttribute::IdentProperty(Ident, Ident) (names bikesheddable)

I do feel like there is a better way that virtue could do derive attribute parsing, but I can't quite put my finger on it. If you have suggestions for a major overhaul please let me know

@VictorKoenders VictorKoenders added the enhancement New feature or request label May 31, 2022
@mkj
Copy link
Contributor Author

mkj commented Jun 1, 2022

I wonder if it would make more sense for it to be something like
parse_tagged_attributes(prefix: &str, atts: &[Attributes]) -> HashMap<Ident, Vec<ParsedAttributeValue>>
rather than having to group and iterate over attributes? That would hide the relative ordering of attributes, I'm not sure if that matters?

Or it could even drop the returned Vec if repeated tags aren't expected?

For Property I think just a IdentProperty is probably fine - I've realised a TokenTree wouldn't be enough anyway if there were something like
#[sshwire(variant = SSH_NAME_ED25519.to_uppercase()]
unless it wrapped the value part in a Group

@kythyria
Copy link

The other missing combination is ParsedAttribute::LiteralTag(Literal). I found myself wanting to parse #[endpoint("/some/where", paginated=offset)].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request not-stale
Projects
None yet
Development

No branches or pull requests

3 participants