Stringing together everything in the command-line interface #39
Conversation
|
I kept this PR as a draft because I still had some unresolved thoughts around various aspects of it. Feel free to discuss whether these are things we need to resolve before merging this PR, or if they can wait until after the first prototype is shipped:
|
|
I will put my comment to your summary first, then will add a particular PR remarks.
I agree.
Is this obsolete? Now you wrote:
Not the first prototype but definitely a good idea! Worth a separate issue :)
Yes, and I think this even in the first version. But not necessarily in this PR. Worth an issue
You mean like default setting of command-line flags? Or something different?
All of them have some issues:
Another idea would be to keep the names of the main modules super simple, indicating only subject they are dealing with, like: Generally, I would like option 4 the most, then 3 ex equo with 1 (common interface argument).
Do we transfer todos to issues?
We can make an issue for it. A lot of beginner-friendly issues may come from this PR :)
👍 |
Still relevant. Even if I seperated things into
Created: #52
Created: #53
Yes. Or at least - if we do it well - there should be a natural mapping between command-line flags and corresponding config directives in
I'll make issues of any TODOs that I don't get around to solving in this PR. |
Ah, what the heck: #54 😆 |
|
Since most of the questions raised above now have their own issues and are independent of this PR, I'll only address the remaining points:
My preference is definitely for the first one. I don't like importing functions (option 2). I also find duplicating the module name in a function name (option 3) does not appeal to my aesthetic taste. For what it's worth, Google's Python style guide states the following:
|
There was a problem hiding this comment.
I enjoyed reading this PR a lot and picked up a bunch of new things :)
The commit history is also so clean and the commit messages helpful.
This was a pleasant experience. 10 out of 10, would recommend!
Will review again for approval once the todo items and the remaining discussion points have been done.
As you said, the mechanism is all there in the action code, so this is more of a design issue. What options are you thinking of for exposing this on the CLI? |
Agree. Our main output should go on stdout. |
jherland
force-pushed
the
jherland/cmdline-with-deps
branch
from
41bdfbd to
e966522
Compare
Thinking maybe of adding two more action options: |
There was a problem hiding this comment.
This is a good interface - simple but to the point. I like it and the simplicity of the code that goes behind :)
I put few comments in the code, and apart from that, I think we need to:
- update README
- consider moving example of projects to conftest for reusability
Side note - running fawltydeps on fawltydeps is excrutiatingly slow. I propose, we add .fawltydepsignore and do not let default search look into directories listed there (like .git, .venv, etc)
Sounds reasonable to me. |
|
@jherland what is your plan for this PR? Are there any CLI changes or tests that you are planning on adding? |
This one I can do in #11. |
jherland
force-pushed
the
jherland/cmdline-with-deps
branch
from
d19f511 to
c3491fc
Compare
Added these in dccf5d3 |
|
From @Nour-Mws:
I think I have added all the stuff I planned for now. Further work/TODOs can be solved in later PRs. From @mknorps:
First draft added in b55d7c5.
I'll leave further reorganizing of test code for a later PR. There are some things I'd like to discuss about what belongs in
Yes. We need some way to ignore files/dirs, and probably even a set of defautl ignores. I don't think we need a |
There was a problem hiding this comment.
Readme instructions are clear and informative.
We can run it later by a person who did not touch this repo, to have user feedback.
Apart from some editorial suggestions, I think this PR is good to be 🚀 :)
Side note - running fawltydeps on fawltydeps is excrutiatingly slow. I propose, we add .fawltydepsignore and do not let default search look into directories listed there (like .git, .venv, etc)
Yes. We need some way to ignore files/dirs, and probably even a set of defautl ignores. I don't think we need a .fawltydepsignore file, though, I'd rather solve it via a fawltydeps.ignore directive in pyproject.toml (see #54).
You are right. pyproject.toml sounds more suited for the job and can be one source of such information.
We're about to import more parts of fawltydeps, in which the function names parse_code(), parse_file(), and parse_dir() are no longer necessarily unambiguous. Instead we want to qualify them with "extract_imports.*" anywhere we use them.
It might seem strange to move what is arguably a command-line interface
detail into the extract_imports module, but this ends up accomplishing
two things:
- Simplify the logic inside the main module. This module is about to
get considerably more complicated with the addition of calls to the
extract_dependencies and check modules.
- Provide a convenience function in extract_imports that smartly
handles whether we pass a file or a directory (or indeed "-" to
signal reading from stdin). A similar function is about to be added
to extract_dependencies, and these will mesh well with how we are
designing the --code (and future --deps) argument in the
command-line interface.
This adds the --deps option (similar to --code) to tell fawltydeps where to look for declared dependencies. It also introduces two new options to choose which action fawltydeps should perform: --list-imports: List the extracted imports and exit --list-deps: List the extracted dependencies and exit When none of these is given, we currently default to --list-imports. The available actions will change soon, as will the default action.
We will soon introduce a new default action to the command-line interface. Change the tests that currently depend on --list-imports being enabled by default to explicitly pass --list-import instead. Also update the test names to reflect this.
This adds the missing piece of the command-line puzzle: a --check option that enables the extracted imports to be compared against the extracted dependencies, and a report to be generated over undeclared and unused dependencies.
When none of --check/--list-imports/--list-deps were given, we used to default to --list-imports. We now instead default to --check, which was always meant to be the main objective for fawltydeps! Add a couple of tests to verify that we do the Right Thing when fewer options are explicitly passed.
When all test cases are named "test_main__*" the "main" signifier is no
longer meaningful. Promote the relevant action option ("list_imports",
"list_deps", or "check") into this prime position instead.
perform_actions() has a lot of considitionals that depend on whether an
action is enabled (i.e. present in the given `actions` set) or not.
These tests were done using a fairly terse syntax:
if actions & {<one or more actions>}:
which is not very readable unless one is already familiar with the
shorthand for set intersections in Python, plus the fact and
non-empty/empty containers evaluate to true/false in a boolean context.
Aid readability by adding an `is_enabled()` inner helper function, which
does exactly the same, but hopefully in a more readable way.
These perform each half of --check. They are mutually exclusive with --check, --list-imports, and --list-deps.
The run_fawltydeps() helper runs our command-line application inside a subprocess. When it fails, we typically want to look at its cpatured stdout/stderr while debugging the test failure. This include the captured stdout/stderr in the test output when run_fawltydeps() fails.
This cuts boilerplate when a test case created a sample project with: - A single Python script that consists of a few import statements - A single requirements.txt that lists a few declared dependencies Instead of manually constructing the files with their needed contents inside each test case, this helper takes a list of imports and a list of declared deps, and then manufactures a code.py with the given imports and a requirements.txt with the given declared deps.
There are a couple of more test cases in test_cmdline.py that manually constructed their own Python projects. Use the newly-merged write_tmp_files() fixture to cut a few lines of boilerplate from these test cases.
With-improvements-from: Maria Knorps <[email protected]>
Co-authored-by: Maria Knorps <[email protected]>
jherland
force-pushed
the
jherland/cmdline-with-deps
branch
from
39d333e to
24bc421
Compare
This PR finally combines
extract_imports,extract_dependenciesandcheckinto a functional command-line interface.There are still a fair amount of sharp corners and TODOs, but this gets us in the ballpark of something that is useable and useful for end users.
main: Rephrase how we import theextract_importsmodule--codeargument intoextract_importsmain: Teach command-line interface to list dependenciestest_cmdline: Prepare for changing default actionmain: Finally teach command-line interface to check imports vs depsmain: Flip default action from--list-importsto--checktest_cmdline: Rephrase test names