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

Change AttrArg workflow - match on name first, type second #121

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

idanarye
Copy link
Owner

No description provided.

@idanarye
Copy link
Owner Author

@ModProg - I've taken your AttrArg idea and modified it so that the name will be matched first. This means that if a setting is used with the wrong style, instead of getting an error that it does not exist we'd get an error that it is of the wrong type. Currently this error does not specify what the correct type is, but that can be added in the future.

@idanarye
Copy link
Owner Author

I'm also considering adding helper functions like iter_if_sub for the other style, but I'm not 100% sure yet if iter_if_sub is the correct abstraction...

@ModProg
Copy link
Contributor

ModProg commented Sep 26, 2023

I think I'd add:

impl AttrArg{
   try_get_value(self) -> Result<Expr, /*Error that says a key value was expected*/>;
   try_get_subcommand<T: Parse>(self) -> Result<impl Iterator<T>, /*Error that says a subcommand was expected*/>;
   try_get_flag(self) -> Result<bool, /*Error that says a flag was expected*/>;
}

as AttrArg knows the ident, it can also set the correct span easily.

@idanarye
Copy link
Owner Author

idanarye commented Sep 26, 2023

My iter_if_sub is similar to your try_get_subattr, but now that I think about it,

for arg in expr.iter_if_sub()? {

is not that much simpler over

for arg in expr.subattr()?.args()? {

So maybe I should just have flag(), keyvalue(), and subattr() methods? The naming convention comes from how Result has ok() and err() methods. The semantics are a bit different (they return Option while I return Result, but the idea is similar.

I also want to have _or_nullify() version of all these methods, which will accept a &mut Option<T> and handle AttrArg::Not.

@idanarye
Copy link
Owner Author

idanarye commented Sep 26, 2023

BTW - I'm thinking that nested would be a better name than sub. I'll keep it sub for now since it's internal API, but I might rename it after #116 gets in.

@idanarye
Copy link
Owner Author

Actually, _or_nullify is a problem because it must still return a value (which does not exist) or an error. Maybe or_not(), which would return and Option?

@ModProg
Copy link
Contributor

ModProg commented Sep 26, 2023

Actually, _or_nullify is a problem because it must still return a value (which does not exist) or an error. Maybe or_not(), which would return and Option?

that makes more sense, I find apis taking &mut to be a bit clunky

@ModProg ModProg mentioned this pull request Sep 26, 2023
@idanarye idanarye merged commit 6960f19 into master Sep 26, 2023
10 checks passed
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.

None yet

2 participants