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

Add pull request and issue sections to Dev Guide #2868

Merged
merged 4 commits into from
Sep 10, 2017
Merged

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Sep 9, 2017

Ref: #2866

@hudochenkov Thanks for digging out those comments. I had completely forgotten about them :)

I’ve tried to streamline the pr and issue processes where I can e.g. I’ve removed the labels which we add to closed issues. Adding these labels is unnecessary work as this info is either:

  1. Communicated in the closing comment e.g. “Best suited as a plugin”, “Dupe of xxxx” etc.
  2. Implicit e.g. the PR was merged after a review, the PR is open and doesn’t have a “PR: needs tests/docs/revision” label and therefore needs reviewing.

The actual documents themselves are pretty terse (and a bit dry I’m afraid) as I wanted to get something up before we add any new members (and it's for a small audience).


I'll also try to make some time over the next few days to help work through the backlog of issues so there's a clean-ish slate for any new members...

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Excellent guides, @jeddy3!

There is small addition related to git history could be helpful.

There is also worth mention, that we need update package-lock.json if we update dependencies. Use latest npm, as it recommended.

- Use the [GitHub review system](https://help.github.com/articles/about-pull-request-reviews/).
- Review against the [Developer Guide criteria](rules.md).
- Assign one or more of the appropriate [`PR: needs *` labels](https://github.com/stylelint/stylelint/labels) when requesting a change.
- Merge simple documentation fixes after one approval and all other PRs after two approvals.
Copy link
Member

Choose a reason for hiding this comment

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

We could also add:

  • Keep git history clean:
    • Start a commit message with uppercase letter.
    • Consider squash and merge commits instead simple merge. It's useful when PR has “Fix lint error,” “Fix typo,” “Update test snapshot”, etc.
    • Use git rebase in your own PRs to avoid unnecessary “Merge ...” commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I've added bits about commit messages, squashing and rebasing.

@jeddy3
Copy link
Member Author

jeddy3 commented Sep 9, 2017

There is also worth mention, that we need update package-lock.json if we update dependencies. Use latest npm, as it recommended.

Isn't the package-lock.json file automatically updated with a npm install package@latest command? If so, I think we don't need to document it... but we can always reassess if it becomes an issue. Although, maybe there is scope for adding another section to the developer guide e.g. prerequisites.md, which details node, npm, dependency updates etc. For another PR though :)

@hudochenkov
Copy link
Member

Isn't the package-lock.json file automatically updated with a npm install package@latest command?

It is. But maintainer should use npm@5 for updating dependencies. Not everybody uses latest npm.

@jeddy3
Copy link
Member Author

jeddy3 commented Sep 9, 2017

It is. But maintainer should use npm@5 for updating dependencies. Not everybody uses latest npm.

Fair point. I've added a "technical prerequisites to contributing" stub.

I also broke the developer guide into sections so that prerequisites make sense (and the user guide for consistency).

@@ -0,0 +1,4 @@
# Technical prerequisites to contributing

- [Node.JS](https://nodejs.org/en/) 8+.
Copy link
Member

Choose a reason for hiding this comment

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

The correct name is “Node.js.” Also, 8+ isn't a legit requirement. Personally, I'm using Node.js 6, and I'm going to switch to Node.js 8 only when it became LTS. I think the most correct version is 4 because it's a minimal version for stylelint itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right.

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

LGTM

@ntwb
Copy link
Member

ntwb commented Sep 10, 2017

This is 👌 , thanks @jeddy3 😄

@hudochenkov
Copy link
Member

BTW, @jeddy3 didn't you delete by accident type: plugin label, when was deleting other labels? Just noticed, that this label is missing #2837 (comment).

@jeddy3 jeddy3 merged commit 975cc2f into master Sep 10, 2017
@jeddy3 jeddy3 deleted the dev-guide-updates branch September 10, 2017 21:32
@jeddy3
Copy link
Member Author

jeddy3 commented Sep 10, 2017

BTW, @jeddy3 didn't you delete by accident type: plugin label, when was deleting other labels? Just noticed, that this label is missing #2837 (comment).

I deleted it as part of streamlining the processes (and therefore what I had to document). No 1. in #2868 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants