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

doc: add text showing that stackoverflow is not managed or moderated … #19416

Closed
wants to merge 1 commit into from

Conversation

JosephLeon
Copy link
Contributor

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 17, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi @JosephLeon! Thanks for the pull request! I'm not sure what the best way to document this is, but I don't think having a parenthetical Not managed by Node.js here and a bold unofficial a few lines below for the IRC channel is the way to do it. At a minimum, it should be consistent.

@Trott
Copy link
Member

Trott commented Mar 17, 2018

@nodejs/documentation

@Trott
Copy link
Member

Trott commented Mar 17, 2018

Maybe the thing to do is to move this to the bullet list below (with the IRC channel) and use unofficial like we do there?

@JosephLeon
Copy link
Contributor Author

JosephLeon commented Mar 18, 2018

Hey @Trott! I moved stack overflow to the unofficial section. I question though if it would be better to remove:

If you didn't find an answer in one of the venues above, you can:

With some heading saying this is the unofficial section and then listing IRC and StackOverflow. My reason for this is there may be more unofficial items to add in the future. Each item would need to be worded to match the above header which doesn't explain the section's content and sets a weird context to follow.

If you agree I can reword that section with some sort of unofficial header and then list IRC and StackOverflow beneath it.

Ex:

If you didn't find an answer in one of the venues above, you can search these unofficial venues:

Then we can remove the bolded unofficial from each item.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Looks good to me as is. I think it would be a little better if StackOverflow were listed first and IRC second. Ideally people would look their before posting in the IRC channel.

@JosephLeon
Copy link
Contributor Author

Moved StackOverflow to be the first item.

README.md Outdated

If you didn't find an answer in one of the venues above, you can:

* View **unofficial** [questions tagged 'node.js' on StackOverflow][]
Copy link

Choose a reason for hiding this comment

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

Sounds weird. Like there are official question, but these ones are unofficial.
Maybe let's move unofficial to the end? Like

Or

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@JosephLeon JosephLeon Mar 18, 2018

Choose a reason for hiding this comment

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

Yes, I agree. Maybe we should reword the heading so we don't have to find ways to embed unofficial in the items. I think the main idea we are trying to convey is that these resources aren't moderated by node.js maintainers.

If you didn't find an answer in one of the official resources above, you can search these unofficial resources which aren't supported by node.js maintainers:

I can always slap unofficial at the end of the StackOverflow line too. I'm open to ideas.

Copy link

Choose a reason for hiding this comment

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

IMO your suggestion is better than the current version. Let's see what @Trott thinks though.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

(Minor nit: node.js -> Node.js. Alternatively, you could stop after unofficial resources and leave the which aren't supported by Node.js maintainers part off. Either way!)

Copy link
Contributor Author

@JosephLeon JosephLeon Mar 18, 2018

Choose a reason for hiding this comment

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

I made the changes @Trott asked for but I'm getting a git error today when I run git push origin 19412:

To github.com:JosephLeon/node.git ! [rejected] 19412 -> 19412 (non-fast-forward) error: failed to push some refs to '[email protected]:JosephLeon/node.git' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Before that I did run:

git fetch upstream
git rebase upstream/master

Which returned Current branch 19412 is up to date.

I was able to push the latest changes using git push origin 19412 -f. Just writing this here in case I should modify my workflow.

Copy link
Member

Choose a reason for hiding this comment

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

@JosephLeon The reason you needed the -f is because rebase is destructive and creates new commits on top of master. So when you push, your code doesn't match the server. I think force push is fine in this case because you're the only one working on your branch.

@benjamingr
Copy link
Member

If you'd like, I answer quite a bit on StackOverflow - and I can talk to their moderation team if we need anything actionable from them.

In general, SO is pretty well moderated in my experience.

Copy link
Contributor

@davisjam davisjam left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Mar 19, 2018

Additional metadata for whoever lands this:

Fixes: #19412

@JosephLeon
Copy link
Contributor Author

@Trott Is there anything else I need to do on this?

@Trott
Copy link
Member

Trott commented Mar 22, 2018

@Trott
Copy link
Member

Trott commented Mar 22, 2018

Fixed lint errors (line wrap at 80 chars in the README), rebased against upstream/master, squashed down to one commit, added metadata.

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/288/

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 22, 2018
Fixes: nodejs#19412

PR-URL: nodejs#19416
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Mar 22, 2018

Landed in 258bcb9

Welcome @JosephLeon and thanks for the contribution! 🎉

@Trott Trott closed this Mar 22, 2018
@JosephLeon
Copy link
Contributor Author

@Trott So when we have a pull request out we should go to https://ci.nodejs.org/job/node-test-pull-request-lite/288/ and make any fixes as noted there? My main reason for doing this particular issue was to get used to the commit pattern with node before taking on a coding issue. I'm reading https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#ci-testing and it seems like only certain people can do this portion. Do you have any additional feedback for me?

@Trott
Copy link
Member

Trott commented Mar 22, 2018

@JosephLeon Hopefully make test (or on Windows vcbuild test) locally will find any issues, but yes, if there are issues in the CI run, then someone needs to figure out what's going on. (Unfortunately, often the problem is unrelated to the change in the pull request.) Only people in the Collaborators group (currently over 100 people, though!) can start CI jobs.

targos pushed a commit that referenced this pull request Mar 24, 2018
Fixes: #19412

PR-URL: #19416
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.