-
Notifications
You must be signed in to change notification settings - Fork 7
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
Yann/feature/filemanagerkit/add exists #1163
Conversation
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections
📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
d59fe09
to
27664f3
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1163 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 147 147
Lines 3789 3798 +9
========================================
+ Hits 3740 3749 +9
Misses 49 49
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Regarding of the solutions, is it still relevant? This PR is paused for now |
27664f3
to
68012df
Compare
Version comparison
|
46eeb1b
to
f9c6a63
Compare
030111d
to
f690531
Compare
638d96d
to
38b52f1
Compare
f690531
to
8b01529
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition! 👍
I've made the suggestion to also provide a function file_does_not_exist
as we are checking for this condition a lot and it would make the code much easier to read :)
auto file = FileManagerKit::File {path, "r"}; | ||
return file.is_open(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested that the file is really closed when it goes out of scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
You can fixup and I'll merge on your go :)
6e3822f
to
564b044
Compare
564b044
to
964c25a
Compare
964c25a
to
1a6db9d
Compare
Kudos, SonarCloud Quality Gate passed! |
Validations
Requirements