Zac-HD / shed

`shed` canonicalises Python code. Shed your legacy, stop bikeshedding, and move on. Black++
https://pypi.org/project/shed/
GNU Affero General Public License v3.0
343 stars 23 forks source link

When processing file lists with globbing characters, warn user #65

Closed tommorris closed 1 year ago

tommorris commented 1 year ago

If you have a moderately complex repository setup (perhaps a monorepo with multiple component subpackages, you're using a VCS other than Git, or you want to combine shed running on Python source code with a linting tool running over non-Python code or other artefacts like XML/JSON/YAML), and seek to manage the complexity of this using a Makefile task like:

format:
    poetry run shed --refactor --py310-plus my_package/**/*.py tests/**/*.py

.PHONY: format

This will fail because Make uses sh which won't expand the globs.

The immediate fix for this is either fixing the Makefile to use find to construct the list of files for processing, or ensuring Make uses a shell other than sh (bash, say).

It might be useful to output a warning if shed has filenames passed in that contain * or ** in them, so the user knows the globs have been passed through without being expanded. Alternatively, perhaps if a list of files is passed through and none of the filenames actually resolve to a Python file, some kind of warning might be useful.

Zac-HD commented 1 year ago

I think printing a "file not found" warning to stderr for each missing file would make sense, and happily that's also easy to implement! Happy to accept a PR; otherwise I'll get to this within the next few weeks.

Zac-HD commented 1 year ago

(misclick! Darn mobile interfaces...)

Cheukting commented 1 year ago

@Zac-HD can this be resolved by adding an extra error message here? https://github.com/Zac-HD/shed/blob/69063295a69afb1e0b9c78798cd428899fc1e590/src/shed/_cli.py#L67

jakkdl commented 1 year ago

Depends on if you want to raise file-missing errors when auto-detecting files in git repos. I can see the case for either way there.

Alternative would be checking all files here https://github.com/Zac-HD/shed/blob/69063295a69afb1e0b9c78798cd428899fc1e590/src/shed/_cli.py#L128-L129

Zac-HD commented 1 year ago

I'd go with Cheuk's solution here - we report an error if git ls-files'd files already, and I don't think adding ", maybe due to unexpanded glob pattern?" is a bad thing in the rather unlikely event that it happens.

Checking after we've failed to open and read the file has two other advantages: it's faster due to our multiprocessing, and it avoids time-of-check/time-of-use errors (which are not much of a concern here, but that way lie subtle mistakes when code changes over time).

Cheukting commented 1 year ago

@tommorris may I know which Make and Bash are you using? I cannot reproduce at mine using:

GNU Make 3.81 and GNU bash, version 3.2.57(1)-release (arm64-apple-darwin22)

shed will run correctly from the make command even with the globs

I can still put in a PR with the extra error message when the file name has '*'s in it, but it would be nice to be able to test it with the problem that it is trying to solve

Zac-HD commented 1 year ago

I'd probably just use a quoted string to get a non-existent path with an asterisk in it - realistic tests are nice for motivation, but not strictly necessary.

jakkdl commented 1 year ago

Oh nvm me, I skimmed too quickly and didn't see how the returned value was handled. For some reason I thought the error message was ignored when running with git ls-files