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

'matcher() and match = should supersede chooser() and choices = `? #263

Closed
Aariq opened this issue Jun 11, 2020 · 3 comments · Fixed by #292
Closed

'matcher() and match = should supersede chooser() and choices = `? #263

Aariq opened this issue Jun 11, 2020 · 3 comments · Fixed by #292
Assignees
Labels
consistent api Uniformity across functions and data sources

Comments

@Aariq
Copy link
Collaborator

Aariq commented Jun 11, 2020

I propose deprecating choices = for all the functions that use it in favor of match = . I know I created choices = and pushed for it, but I think I like the behavior of match = better now, and it would bring some consistency to the package.
I feel a little bad for the confusion this would introduce (deprecating first in favor of choices, now deprecating choices in favor of match), but it seems worth it maybe.

@Aariq
Copy link
Collaborator Author

Aariq commented Jun 11, 2020

affected functions would include:

  • cts_convert()
  • pc_synonyms()
  • cir_query()

In addition, get_etoxid() uses the chooser() function internally, although it has a match argument. It should probably be switched to use the matcher() utility function and then we can completely get rid of the chooser() utility.

@Aariq Aariq added the consistent api Uniformity across functions and data sources label Jun 11, 2020
@stitam
Copy link
Contributor

stitam commented Jun 15, 2020

That functionality was kind of evolving over time. I think it's fine that we experimented with it and I agree that match is probably the best approach we currently have. If you want to make this more consistent, feel free to go ahead, and thanks!

@Aariq Aariq self-assigned this Jun 19, 2020
Aariq added a commit to Aariq/webchem that referenced this issue Jun 23, 2020
stitam added a commit that referenced this issue Jul 8, 2020
Check coverage across webchem. Closes #262, #263.
@Aariq
Copy link
Collaborator Author

Aariq commented Jul 9, 2020

I think I got all the instances of this. chooser() is still in utils.R though, so I'll leave this open until we remove it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistent api Uniformity across functions and data sources
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants