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

Remove Option from NoteGetterOption::select #8413

Closed
nventuro opened this issue Sep 6, 2024 · 1 comment · Fixed by #8504
Closed

Remove Option from NoteGetterOption::select #8413

nventuro opened this issue Sep 6, 2024 · 1 comment · Fixed by #8504
Assignees

Comments

@nventuro
Copy link
Contributor

nventuro commented Sep 6, 2024

The select function has the following signature:

pub fn select<T>(
    &mut self,
    property_selector: PropertySelector,
    value: T,
    comparator: Option<u8>)

If the option is none, it defaults to EQ. This makes for cryptic code, e.g.:

options.select(NFTNote::properties().token_id, token_id, Option::none()).set_limit(1);

I think we should remove this option and make the comparator mandatory, so the above would then become:

options.select(NFTNote::properties().token_id, token_id, EQ).set_limit(1);

which is much more reasonable. We could even switch the order of the params, like we did in #8384, so that it's lhs, op, rhs.

@benesjan
Copy link
Contributor

benesjan commented Sep 6, 2024

Having lhs, op, rhs would be nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants