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.

Sign up for GitHub

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

Jump to bottom

doc,test: minor improvements to O_DSYNC #15547

Closed
wants to merge 2 commits into from
Closed

doc,test: minor improvements to O_DSYNC #15547

wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

synchronous is misleading here, it should be synchronized. Also simplified the test.

Refs: #15451

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs, doc, test

addaleax
addaleax approved these changes Sep 22, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, the only suggestion I’d have is to use doc,test: for the commit message (because the primary use for the subsystem label is to give users the ability to make out functionality changes to the built-in modules in the changelogs)

cjihrig
cjihrig approved these changes Sep 22, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I don't really see the point in changing the test, but LGTM.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Sep 22, 2017
BridgeAR
BridgeAR approved these changes Sep 24, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I also do not see the reason in changing the test. If at all changing the test the assert.ifError might be used as direct callback as that is the original intention behind that assert function.

I also agree that the subsystem should be changed but this can also happen during landing.

@tniessen tniessen added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Sep 24, 2017
@tniessen tniessen changed the title fs: minor improvements to O_DSYNC doc,test: minor improvements to O_DSYNC Sep 24, 2017
@tniessen
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/10241/

tniessen added a commit to tniessen/node that referenced this pull request Sep 26, 2017
@tniessen
doc,test: minor improvements to O_DSYNC
f98687d 
PR-URL: nodejs#15547
Refs: nodejs#15451
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@tniessen
Copy link
Member Author

Landed in f98687d.

@tniessen tniessen closed this Sep 26, 2017
@MylesBorins
Copy link
Contributor

This is not landing cleanly on 8.x, should it be backported?

@tniessen tniessen mentioned this pull request Sep 28, 2017
Closed
2 tasks
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
@tniessen @addaleax
doc,test: minor improvements to O_DSYNC
a53d961 
PR-URL: nodejs/node#15547
Refs: nodejs/node#15451
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Oct 3, 2017
@tniessen
doc,test: minor improvements to O_DSYNC
9d97fcc 
PR-URL: nodejs#15547
Refs: nodejs#15451
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 7, 2017
@tniessen @MylesBorins
doc,test: minor improvements to O_DSYNC
dec383a 
Backport-PR-URL: #15653
PR-URL: #15547
Refs: #15451
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 7, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@tniessen @MylesBorins
doc,test: minor improvements to O_DSYNC
6003afc 
Backport-PR-URL: #15653
PR-URL: #15547
Refs: #15451
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tniessen @MylesBorins @addaleax @lpinca @cjihrig @BridgeAR @mscdex