Use Click to parse command-line arguments in lemke.py#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the lemke CLI in src/lemke/lemke.py from manual sys.argv parsing to Click, aiming to improve help/usage output and validate the input file path up front.
Changes:
- Replaced manual argument parsing with a Click-based
main()command (options +lcpfilenameargument). - Updated
tableau.runlemke()to acceptlcpfilenameand derive the.outfilename from it. - Added
clickas a runtime dependency inpyproject.toml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/lemke/lemke.py |
Introduces Click CLI decorators/options/argument parsing and threads lcpfilename into runlemke() for .out generation. |
pyproject.toml |
Adds Click as a project dependency to support the new CLI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "lcpfilename", | ||
| default="lcp", | ||
| required=False, | ||
| type=click.Path(exists=True, readable=True), |
| outfile = lcpfilename + ".out" | ||
|
|
||
| if silent: | ||
| filehandle = open(outfile, "w") # noqa: SIM115 | ||
| n = self.n |
| printout(f"verbose={verbose} lcpfilename={lcpfilename} silent={silent} z0={z0}") | ||
| # printout (f"{verbose}= {lcpfilename}= {silent}= {z0}=") | ||
| m = lcp(lcpfilename) | ||
| printout(m) | ||
| printout("==================================") | ||
| tabl = tableau(m) | ||
| tabl.runlemke(verbose=verbose, z0=z0, silent=silent) | ||
| tabl.runlemke(verbose=verbose, z0=z0, silent=silent, lcpfilename=lcpfilename) |
| def main(): | ||
| processArguments() | ||
| @click.command( | ||
| context_settings={"help_option_names": ["-?", "--help"]}, |
There was a problem hiding this comment.
add "-h" as another help option name
| outfile = lcpfilename + ".out" | ||
|
|
||
| if silent: | ||
| filehandle = open(outfile, "w") # noqa: SIM115 |
There was a problem hiding this comment.
I think when silent==True there should not be any output at all (except for error messages),
including no output to file. This will also avoid the open-file handle leak risk noted by copilot.
The standard output to file is recording the pivots, if verbose==True also the intermediate tableaus.
We have to look at lemke.py as well as check for further parameters, later.
There was a problem hiding this comment.
So, when silent is True, there should be no output at all (neither to a file, nor to the console). But when silent is False, should we print to both, or only to a file?
stengel
left a comment
There was a problem hiding this comment.
looks very good. See my suggestions, e.g. from copilot - reject directories,
and no output if silent==True.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Some guerrilla comments on instrumenting the command-line tool, based on how command-line tools typically work:
Specifically for Python:
Looking ahead, a logical refactoring step is going to be to have callback functions to the main lemke routine, which will be the mechanism where you'll actually write to output if desired - you won't have any printing going on in lemke proper. But that is a few steps down the road - but the above will move in the direction of making that easy. |
|
Don't allow a default for the input filename - it should be a required argument.
You might consider allowing "-" as an input filename
I would suggest NEVER suppress output to stdout. If someone really wants to suppress that then you pipe to /dev/null.
If you allow a logfile to be generated, the logfile might be more detailed than the stdout output.
Think about all of the things you might include in/include out of a detailed logfile
Try very hard to use context managers for opening files
and the question is how to use that together with click which is in effect the main program calling lemke when using the CLI. There must be a standard solution for this. |
|
When this is used as a library we will have gotten to the point where all of the writes are done by callbacks, as I noted in my previous comment, so Being able to read from standard input can be very useful so you don't have to create temporary files. Filename suffixes shouldn't be assumed, they should be specified explicitly on the command line - leave it to the user how they want to do it. Using a context manager is straightforward, wrap the call to with open(output_filename, "w") as f:
runlemke(<arguments, which would include f as the output stream>)Eventually we will just pass callback(s) to |
|
Here are the recent changes I made (although not sure if this is what you had in mind):
|
| class tableau: | ||
| # filling the tableau from the LCP instance Mqd | ||
| def __init__(self, Mqd): | ||
| self.output_stream = None |
There was a problem hiding this comment.
Storing the stream as state in the tableau is not great. However, this is more of a symptom of a deeper problem than a problem in itself - the tableau class mixes confuddles up the tableau object with the lemke algorithm, which are logically distinct. Further refactorings will solve this, so I think this is OK for now because the current commit is about getting click to be responsible for the command-line parsing.
| version = "0.0.1" | ||
| dependencies = [ | ||
| "numpy>=2.2,<2.3", | ||
| "matplotlib>=3.10,<3.11" |
There was a problem hiding this comment.
Upper bounds on packages should only be used in the event of known incompatibilities, especially when working with mature packages (like numpy, matplotlib, click all are). Upper bounds can cause version conflicts when installing alongside other packages in the same environment.
This PR replaces manual command-line argument parsing with Click (in
lemke.py).Changes in detail
main()accepts arguments (populated by Click), which are then passed ontorunlemke().lcpfilenameargument was not a valid file path, the program used to raise an unhandled exception. Now, this argument is checked by Click and if the file does not exist, or is not readable, a clean CLI error is shown automatically.-v) and can be combined (-vs), while multi-character flags use double dashes (--verbose) to avoid ambiguity.