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 #195

Merged
merged 5 commits into from
Oct 9, 2020
Merged

Conversation

thomashoneyman
Copy link
Contributor

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, there's one more step to take. I applied the standard eslint configuration, which was initially developed using the rules from this repository and covers the same rules from the jshint configuration currently here. This configuration is a little stricter and has caught several (potential) issues in the code:

  • Several unused variables, including FIBER, THUNK, some local variables (ie. "i")
  • Several unused parameters (ie. 'function (unused) { ... }`)
  • (According to eslint) some unsafe uses of variables when defining functions in loops, according to the no-loop-func rules

These merit an independent discussion and review aside from this PR, which doesn't touch the code other than to disable eslint so we can have that separate discussion.

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 👍

Should we open an issue for cleaning up the eslint errors?

@thomashoneyman
Copy link
Contributor Author

I'd like to briefly check in with @natefaubion and hear how he'd like to handle it. I think ideally we would merge this pull request, then open a PR which addresses the eslint warnings and errors -- perhaps modifying the eslint configuration if needed. @natefaubion knows much, much more about the Aff internals than I do so this configuration should really come down to his decision.

@natefaubion
Copy link
Collaborator

As far as eslint goes, I would prefer to keep the source as is and not make unnecessary stylistic changes just for the sake of it. If there ever comes a time to do significant refactoring we can take that as an opportunity to be more consistent.

@thomashoneyman
Copy link
Contributor Author

The eslint rules which would otherwise cause a failed build have been disabled in the source (but the configuration has been kept very close to the standard one). We can update the source itself when there is reason to.

@thomashoneyman thomashoneyman merged commit 2b9f608 into main Oct 9, 2020
@thomashoneyman thomashoneyman deleted the contributors-update branch October 9, 2020 22:17
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.

3 participants