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

Update for Contributors library guidelines #176

Merged
merged 5 commits into from
Oct 10, 2020
Merged

Conversation

thomashoneyman
Copy link
Contributor

@thomashoneyman thomashoneyman commented Oct 9, 2020

This pull request is part of an effort to update and standardize the Contributors libraries according to the Library Guidelines. Specifically, it:

  1. Adjusts the files and repository structure according to the repository structure section of the guidelines, which includes standard pr templates, issue templates, CI in GitHub Actions, ensures the project uses Spago, and so on.
  2. Updates the README and documentation according to the documentation section of the guidelines. This is a first step towards ensuring Contributors libraries have adequate module documentation, READMEs, a docs directory, and tests (even if just usage examples) in a test directory.

In this library's case, I've taken a few specific actions.

  1. I moved the bulk of the README documentation into the docs directory and linked to it from the README, for parity with how all the other contrib-libraries are handling this.
  2. I applied the standard eslint configuration, which caught a few issues.

This PR isn't intended to change the code whatsoever, just to make sure that this library is using the same configurations and processes as the other libraries in this organization. With that in mind, despite adding the eslint configuration, this configuration is currently disabled so that it can be evaluated separately in a followup PR.

src/React.js Outdated Show resolved Hide resolved
src/React.js Outdated Show resolved Hide resolved
src/React/DOM/Props.js Outdated Show resolved Hide resolved
main :: Effect Unit
main = do
log "🍝"
log "You should add some tests."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware this test does nothing at the moment, but we may (in the future) want to write something small here which minimally exercises the bindings as a sanity check when making future changes. Having this directory ready to go and running in CI makes it easier for contributors to help add tests in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the right call. Having the test structure in place will definitely make for a smoother experience if someone wants to add some tests.

src/React.js Outdated Show resolved Hide resolved
src/React/DOM/Props.js Outdated Show resolved Hide resolved
src/React.js Outdated Show resolved Hide resolved
@maxdeviant
Copy link
Member

@thomashoneyman I'm wondering if we should suppress the eslint warnings for the purposes of this PR and then come back and remove the suppressions and fix the offending code in a separate PR?

@thomashoneyman
Copy link
Contributor Author

That's a better idea. Thanks for the suggestion.

"react": "^16.3.0"
"private": true,
"scripts": {
"build": "spago build --purs-args '--censor-lib --strict'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a subsequent PR we can enable eslint via eslint src && spago build ... here.

Copy link
Member

@maxdeviant maxdeviant left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@thomashoneyman thomashoneyman merged commit e9e801a into main Oct 10, 2020
@thomashoneyman thomashoneyman deleted the contrib-update branch October 10, 2020 21:54
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

2 participants