Refactor PDBList CLI to use argparse while preserving legacy behavior#5167
Refactor PDBList CLI to use argparse while preserving legacy behavior#5167MANICHELLURII wants to merge 3 commits intobiopython:masterfrom
Conversation
for more information, see https://pre-commit.ci
|
I would appreciate feedback on whether the updated CLI structure aligns with current Biopython conventions and if any additional backward compatibility concerns should be addressed. Please let me know if dedicated CLI tests are expected or if the argument-handling logic would be better placed in a separate entry-point script. I’m happy to refine the implementation further. |
| parser.add_argument( | ||
| "action", | ||
| help="update | all | obsol | assemb | PDB ID | (PDB1,PDB2,...)" | ||
| ) |
There was a problem hiding this comment.
You can't list the valid actions here can you (in plain argparse)? I've been using a more fancy command line API lately so my expectations have changed 😅
There was a problem hiding this comment.
@peterjc it was my first PR so it took me somewhile to understand properly & i can resolve above issue clean & clear,
thank you for the review.
There was a problem hiding this comment.
There isn't a better way here because while the 'named actions' (all, update, etc) can be checked, the wildcard PDB-ID arguments cannot.. The correct way here would be to move all those named actions to options and make the only argument pdb ids, but that breaks the current interface.
I'm OK doing that and it's a much better QoL improvement, but it'll need a deprecation warning for a couple of releases.
|
You'll need to run black on this. Otherwise, at first skim-read you've done a nice job: this looks like a faithfull backward compatible re-write using |
| parser.add_argument( | ||
| "action", | ||
| help="update | all | obsol | assemb | PDB ID | (PDB1,PDB2,...)" | ||
| ) |
There was a problem hiding this comment.
There isn't a better way here because while the 'named actions' (all, update, etc) can be checked, the wildcard PDB-ID arguments cannot.. The correct way here would be to move all those named actions to options and make the only argument pdb ids, but that breaks the current interface.
I'm OK doing that and it's a much better QoL improvement, but it'll need a deprecation warning for a couple of releases.
|
|
||
| args = parser.parse_args() | ||
| # Backward compatibility for legacy format flags | ||
| if args.pdb: |
There was a problem hiding this comment.
I'd slap a deprecation warning on whoever is using these flags and direct them to the --format option. Otherwise we'll live with this forever.
Summary
This PR replaces manual
sys.argvparsing inBio/PDB/PDBList.pywithargparse.Improvements
argparse--helpgenerationepilog-pdb-xml-mmtfupdate,all,obsol,assemb(1ABC,2XYZ)--with-assembliesMotivation
Improves maintainability, clarity, and future extensibility of CLI handling while preserving existing behavior.