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 maintainers guide #272

Merged
merged 3 commits into from
Oct 3, 2019
Merged

Conversation

shaydewael
Copy link
Contributor

Summary

Adds information about maintaining the package and addresses #229

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #272 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   59.95%   59.95%           
=======================================
  Files           7        7           
  Lines         492      492           
  Branches      136      136           
=======================================
  Hits          295      295           
  Misses        169      169           
  Partials       28       28

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac9720b...7245605. Read the comment docs.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for making the great guide!


### Testing

This package has unit tests for most files in the same directory the code is in with the suffix `.spec` (i.e. `exampleFile.spec.ts`). You can run the entire test suite using the npm script `npm test`. This command is also executed by Travis, the continuous integration service, for every Pull Request and branch. The coverage is computed with the `codecode` package. The tests themselves are run using the `mocha` test runner.
Copy link
Member

Choose a reason for hiding this comment

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

Considering the following statement,

Test code should be written in syntax that runs on the oldest supported Node.js version, without transpiling.

the example file name exampleFile.spec.ts needs to be exampleFile.spec.js.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no. All the tests in this project are written in TS. Therefore, just removing the part , without transpiling would be fine. 👍

The documentation is built using [Jekyll](https://jekyllrb.com/) and hosted with GitHub Pages.
The source files are contained in the `docs` directory. They are broken up into the `_basic`, `_advanced`, and `_tutorials` directories depending on content's nature.

All documentation contains [front matter](https://jekyllrb.com/docs/front-matter/) that indicates the section's title, slug (for header), respective language, and if it's not a tutorial it contains the order it should appear within it's respective section (basic or advanced).
Copy link
Member

Choose a reason for hiding this comment

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

Also, the maintainers need to be aware of also updating Japanese translation (plus we may have other languages as well in the future) when they update documents.

I'm still not sure what the most easy-to-understand way to communicate regarding this matter for contributors is but one thing I can suggest is to have a more comprehensive checklist with categories (e.g., code change, documents, etc) in .github/pull_request_template.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think pull_request_template.md may be a good place to indicate this. Maybe we could add a label for documentation that is Needs Translation, but I also don't want to wait to merge PRs that aren't translated yet since that could take some time.

@shaydewael shaydewael merged commit 1f551bf into slackapi:master Oct 3, 2019
@shaydewael shaydewael deleted the add-maintainers-guide branch October 3, 2019 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants