Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tommorris opened this issue Jan 25, 2023 · 8 comments · Fixed by #99
Closed

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

tommorris opened this issue Jan 25, 2023 · 8 comments · Fixed by #99

Comments

@tommorris
Copy link

tommorris commented Jan 25, 2023

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
Copy link
Owner

Zac-HD commented Jan 26, 2023

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 Zac-HD closed this as completed Jan 26, 2023
@Zac-HD
Copy link
Owner

Zac-HD commented Jan 26, 2023

(misclick! Darn mobile interfaces...)

@Zac-HD Zac-HD reopened this Jan 26, 2023
@Cheukting
Copy link
Contributor

@Zac-HD can this be resolved by adding an extra error message here?

except (OSError, UnicodeError) as err:

@jakkdl
Copy link
Contributor

jakkdl commented Jun 19, 2023

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

shed/src/shed/_cli.py

Lines 128 to 129 in 6906329

if args.files:
all_filenames = args.files

@Zac-HD
Copy link
Owner

Zac-HD commented Jun 19, 2023

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
Copy link
Contributor

Cheukting commented Jun 20, 2023

@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
Copy link
Owner

Zac-HD commented Jun 20, 2023

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
Copy link
Contributor

jakkdl commented Jun 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants