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

Check if path exists before creating FileInfo #8840

Merged
merged 1 commit into from Feb 10, 2021

Conversation

gerhardol
Copy link
Member

Will avoid exceptions for illegal characters etc

Fixes #8839

Proposed changes

Use File.Exists(path) before FileInfo(path) to avoid exceptions.

Can be seen as a follow up to #8575

Test methodology

review


✒️ I contribute this code under The Developer Certificate of Origin.

Will avoid exceptions for illegal characters etc
@ghost ghost assigned gerhardol Feb 9, 2021
@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #8840 (799ae63) into master (b69f89e) will increase coverage by 0.01%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #8840      +/-   ##
==========================================
+ Coverage   56.10%   56.11%   +0.01%     
==========================================
  Files         919      919              
  Lines       65522    65521       -1     
  Branches    11997    11996       -1     
==========================================
+ Hits        36760    36770      +10     
+ Misses      25759    25752       -7     
+ Partials     3003     2999       -4     
Flag Coverage Δ
production 43.27% <25.00%> (+0.02%) ⬆️
tests 94.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

LGTM with a question

Comment on lines -2470 to -2474
if (string.IsNullOrWhiteSpace(filePath))
{
continue;
}

Copy link
Member

Choose a reason for hiding this comment

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

All other changes make sense, but this change is surprising in the given context. Could you please elaborate on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

null annotation is coming soon...
null is an acceptable input with the change so the caller do not need to check

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 10, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 10, 2021
@RussKie RussKie merged commit a28d70b into gitextensions:master Feb 10, 2021
@ghost ghost added this to the 3.6 milestone Feb 10, 2021
@RussKie RussKie modified the milestones: 3.6, 3.5 Feb 10, 2021
@gerhardol gerhardol deleted the feature/fileinfo-exception branch February 10, 2021 11:28
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.

[NBug] The given path's format is not supported.
2 participants