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

Return empty tuple for no selected files #415

Merged
merged 4 commits into from
Jun 8, 2022

Conversation

aeisenbarth
Copy link
Contributor

This change makes the FileEdit in EXISTING_FILES mode return an empty tuple if no files are selected.

The ability to detect "no selected file" is important to prevent parsing invalid/non existing files. The FileEdit has three possibilities, either it always has a Path value, or it is nullable and the neutral value should be None, or it is in multiple file selection mode and according to the signature the neutral value should be a tuple of zero length.

However, since "".split(some_delimiter) always gives [""] instead of [], it currently returns a tuple with an empty path Path(".") which equals the home directory and is rather impractical to distinguish from an intentionally selected path.

This change makes the FileEdit in EXISTING_FILES mode return an empty tuple if no files are selected.

The ability to detect "no selected file" is important to prevent parsing files that don't exist. The FileEdit has three possibilities, either it always has a Path value, or it is nullable and the neutral value should be None, or it is in multiple file selection mode and according to the signature, the neutral element should be an empty tuple.

However, since `""`.split(some_delimiter)` always gives `[""]`, it currently returns a tuple with an empty path `Path(".")` which equals the home directory and is very impractical to distinguish from an intentionally selected path.
@tlambert03
Copy link
Member

thanks! can you add a test somewhere? Also, what about tuple(Path(p) for p in text.split(", ") if p.strip()) ... which should additionally catch any "" that happen to sneak through with other valid paths?

@aeisenbarth
Copy link
Contributor Author

Good point! I was looking at the code that explicitly sets the line edit to an empty string. My version wouldn't have caught spaces " ". I don't see yet how that could happen other than by user input (but then there would be more cases to consider, like files separated by extra spaces ", " and when stripping those we can't support file paths ending with space and I believe file paths containing comma and space would already mess it up).

@aeisenbarth
Copy link
Contributor Author

I also fixed two more issues:

  • For a single (nullable) path, "" would be expanded to an absolute path, the same for multiple mode with an empty files list.
  • For multiple files, only the first path was expanded, since the expansion was applied after joining to a single string.

Also, is str still allowed for assignmend? The mypy hook complained because the annotation is Path (although the TypeError in the code allows str). Should I change annotations to PathLike?

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #415 (9fcc753) into main (e383926) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   88.74%   88.90%   +0.15%     
==========================================
  Files          30       30              
  Lines        3838     3839       +1     
==========================================
+ Hits         3406     3413       +7     
+ Misses        432      426       -6     
Impacted Files Coverage Δ
magicgui/widgets/_concrete.py 86.65% <100.00%> (+1.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e383926...9fcc753. Read the comment docs.

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

thanks! this looks great! and thank you for the tests!

The mypy hook complained because the annotation is Path (although the TypeError in the code allows str). Should I change annotations to PathLike?

which annotation? in the getter? (things look good to me as is)

@tlambert03 tlambert03 merged commit a2b1dba into pyapp-kit:main Jun 8, 2022
@tlambert03 tlambert03 added the bug Something isn't working label Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants