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

User messages for missing files #1863

Merged
merged 16 commits into from
May 5, 2022
Merged

Conversation

toastisme
Copy link
Collaborator

@toastisme toastisme commented Aug 25, 2021

This adds a warning for dials.import if no input files are found when using a wildcard, and an error message if a specific requested file is missing.

Currently the behaviour when using wildcards is to show help if no files are found, or when using multiple wildcards to show nothing if some wildcards return nothing. This adds a warning to the user if a wildcard returns nothing.

The current behaviour for when a specific file is not found is to show the traceback from dxtbx. This pr hides the traceback and shows just the file not found error, followed by calling help.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #1863 (34ff460) into main (120cd59) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1863      +/-   ##
==========================================
- Coverage   80.36%   80.36%   -0.01%     
==========================================
  Files         580      580              
  Lines       65698    65706       +8     
  Branches     9259     9260       +1     
==========================================
+ Hits        52800    52805       +5     
- Misses      10844    10847       +3     
  Partials     2054     2054              

@toastisme toastisme changed the title Add warning if no input files are found when using wildcard User messages for missing files Aug 25, 2021
util/options.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful to add a simple test case to cover this (although possibly overkill)?

newsfragments/1863.bugfix Outdated Show resolved Hide resolved
util/options.py Outdated Show resolved Hide resolved
util/options.py Outdated Show resolved Hide resolved
toastisme and others added 2 commits September 8, 2021 12:13
util/options.py Outdated Show resolved Hide resolved
@toastisme
Copy link
Collaborator Author

Might be useful to add a simple test case to cover this (although possibly overkill)?

Not quite sure what this would be testing if it's just logger statements based on Glob and a FileNotFoundError. I guess you could have a test to confirm that trying to import a non-existent file always gives a logger.error followed by calling help, and trying to import multiple wildcards where one returns nothing gives only a logger.warning. Does feel a bit overkill to me..

@ndevenish
Copy link
Member

ndevenish commented Oct 29, 2021

I think part of the confusion is that control flow here is rather non-obvious - this prints the message, but doesn't mark it as unhandled. In dials.import this is only called to parse images, so no experiment is ever created. dials.import then sees that there are no experiments, and prints the help.

I don't think printing help is particularly helpful in this case, because there was (otherwise looking valid) input, and the giant block of text somewhat hides the actual error message. If the user asked for a file and it wasn't present, this seems a somewhat reasonable case to call sys.exit()?

What other downstream scenarios are there that a missing file here is a recoverable error?

(Otherwise, this is a measurable improvement over a stack trace)

@toastisme
Copy link
Collaborator Author

toastisme commented Nov 3, 2021

Sorry this had slipped my mind. Yes I agree the solution isn't ideal. It would be more consistent for missing files to be caught in unhandled. My original reason for not doing this was that it's not particularly helpful for the user to see an argument is unhandled but not know it's due to missing files.

A better solution might be changing unhandled arguments in general to be a dictionary which explain the reason for each unhandled argument (99% of which I'm guessing would just be unrecognised arguments).

@ndevenish
Copy link
Member

Even though I think we could do better here, this is absolutely an improvement to what we had before.

@ndevenish ndevenish enabled auto-merge (squash) May 4, 2022 14:59
The code changed '<image files>' to always be appended to the
experiment list, even if there were no image files passed.
@ndevenish ndevenish disabled auto-merge May 5, 2022 09:54
@ndevenish ndevenish enabled auto-merge (squash) May 5, 2022 09:55
@ndevenish ndevenish merged commit c645ecc into main May 5, 2022
@ndevenish ndevenish deleted the wildcard_files_not_found_warning branch May 5, 2022 12:29
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 this pull request may close these issues.

4 participants