Avoid code repetition among import and import_list #16
Conversation
src/pyorcidator/import_info.py
Outdated
| qs = render_orcid_qs(orcid) | ||
| @click.option( | ||
| "--orcids", | ||
| prompt="ORCID ID or a path to list of ORCID IDs", |
There was a problem hiding this comment.
HIGHLY suggest against doing this. Each option in a non-insane CLI should correspond to one datatype only. Just make two different options or two different CLIs if they have different semantics
There was a problem hiding this comment.
I do not have enough experience here; I'll just leave it open for now
There was a problem hiding this comment.
That's actually a great point, @cthoyt. I've read the same advice before but it still didn't cross my mind. My main worry in this case is to avoid unnecessary code repetition, but I'll refactor it as soon as I can. Thank you!
- Instead of repeating code between import info from list and import info, include a type guard on import_info to check if the argument provided is a path or a just a string. - Addresses lubianat#14
- Keeps both commands, but refactors repeated functionality into a separate function. - Also improves the way the path to the ORCID file is read - Also adds missing open_browser to import_list - Closes lubianat#14
- This way the same option code isn't repeated between modules, it's merely passed down as context - Open browser can now be called as: `pyorcidator --open-browser import --orcid ORCID_ID`
|
Following the great suggestion by @cthoyt I've kept the options separate but I've also kept them as separate commands - while externalizing the code they shared to a separate utility function, just so the code isn't repeated, which was my original intention. I'll change this PR's title to reflect these updates. |
jvfe
changed the title
Instead of repeating code between import info from list and import
info, externalize the code they share to a separate function - so it's more easily maintainable in the long run.
Also uses click.echo instead of print()
Closes Avoid code repetition between import_info and import_info_from_list #14