-
Notifications
You must be signed in to change notification settings - Fork 155
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
Fix Issue 747 #753
Fix Issue 747 #753
Conversation
…alidate_flags function
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
=========================================
- Coverage 86.88% 86.8% -0.09%
=========================================
Files 16 16
Lines 1739 1781 +42
=========================================
+ Hits 1511 1546 +35
- Misses 228 235 +7
Continue to review full report at Codecov.
|
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.
Nice work on this. After I get my grades submitted, I'll spend some more time on this, but in the meantime, I've left some feedback. I think this is pretty close to be ready.
One thing we could do to shorten the code in implementation.js
would be to do the following at the top after Constants
is required (~line 8):
const {
O_RDONLY,
O_WRONLY,
O_RDWR,
O_CREAT,
O_TRUNC,
O_APPEND
} = Constants.fsContansts;
Then we can get rid of all the Constants.fsConstants.*
when you just want to use something like O_RDONLY
. Make sense?
@@ -1633,6 +1663,7 @@ function validate_file_options(options, enc, fileMode){ | |||
} | |||
|
|||
function open(context, path, flags, mode, callback) { | |||
flags = stringToFlags(flags); |
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.
Let's move this down to be with the other flags validation happening on 1691
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.
I ended up moving that line, along with the validation, up a little further in the function -- flags
gets used prior to line 1691, but until this most recent change, wasn't actually validated until that line. I figured it would make more sense to validate them almost immediately, or at least do that before they're used. Thank you for pointing this out, though -- I don't think I would have caught that mistake if it weren't for this suggestion!
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.
I think this looks good, excellent work. I don't have time to test it today, but will try to do so in the next few days, and can likely land it then. The code seems right to me.
Thanks for finishing this!
Fixes issue #747 by allowing fs.open flags to be in the form of a string or bitwise number. Adds a single test for creating a file using
fsConstants.O_CREAT
.