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

New Relic Repolinter Improvements #174

Merged
merged 112 commits into from
Sep 30, 2020
Merged

Conversation

prototypicalpro
Copy link
Collaborator

Motivation

This PR addresses notes mentioned in #173.

Proposed Changes

Pulling from the newrelic-forks repolinter changelog:

Breaking

  • The ruleset configuration format has been upgraded to version 2, including adding a JSON schema and support for YAML. The previous ruleset format is still supported (albeit not as well tested).
  • Renamed several rule options to more clearly convey functionality (files -> globsAny) and remove problematic language (blacklist -> denylist). Backwards compatibility for old option names in version 1 rulesets is still maintained, however the schema will (at the moment) fail to validate the old names in version 2.
  • Major changes have been made to the lint function:
    • Formatting and printing have been moved outside lint, allowing the developer to suppress or modify the output as needed. This change is reflected in the new CLI implementation.
    • The object returned by lint (LintResult) has been completely restructured.
    • async was added to the interface.
  • Major changes have been make to the JSON Formatter to accommodate the structure change of lint's return value.
  • Non top-level configuration support (ex. targetdir/otherdir/repolinter.json would trigger another lint of otherdir) has been removed for now. I'd be open to adding this functionality back before this PR is merged.
  • Some slight changes have been made to the default formatter to accommodate the feature list below.

Features

  • Automatic fixes have been added. These fixes must be configured in your ruleset before they can be used, but are otherwise enabled by default.
  • Markdown formatting is now supported via a CLI argument.
  • CLI argument parsing has re-implemented with Yargs to allow for a more user-friendly experience. All previous commands and arguments remain; however, several new options are now available. For more information on these options please see the Repolinter CLI.
    • --dryRun/-d - Disable fixes.
    • --allowPaths/-a - Specify an allowlist that repolinter should limit itself to.
    • --rulesetFile/-r - Manually specify the configuration repolinter should use.
    • --rulesetUrl/-u - Specify a URL where repolinter can retrieve the ruleset from.
    • --format/-f - Change the output format.
  • Added several other functions to the Node API: runRuleset, determineTargets, validateConfig, and parseConfig.
  • Added TypeScript types for the Node API.

Fixes

  • All file-based operations have been moved to fs.promises, which increased performance quite a bit.
  • Fixed some issues with Windows paths.
  • Added more tests and autogenerated documentation.
  • Modified the build action to run tests on Windows and MacOS. Added a documentation action.
  • Added an ESLint config which lints StandardJS and JSDoc comments. This config needs work as the JSDoc plugin for eslint doesn't understand some of the syntax used.
  • Updated NPM dependencies.

Notes

  • The autogenerated documentation is currently deployed to Github Pages, which won't transfer over when this PR is merged. I'm not sure if this is the correct place for it and am open to suggestions.
  • Same goes for the typescript types, which currently reside with the repository.

Due to the scope of the aforementioned changes, we also propose bumping the version of Repolinter to v1.0.0 after this PR is merged.

Please feel free to suggest modifications, or point out something I missed. Thanks again!

@prototypicalpro
Copy link
Collaborator Author

It looks like the contributor count axiom PR conflicts with my changes. I'll see if I can add that feature back into our fork so this will merge cleanly.

@caniszczyk
Copy link
Member

@prototypicalpro no worries it's a small change so hopefully not too bad

@prototypicalpro
Copy link
Collaborator Author

I've merged the contributor-count axiom in, and this PR should be good to go.

@caniszczyk
Copy link
Member

caniszczyk commented Aug 31, 2020 via email

@prototypicalpro
Copy link
Collaborator Author

I did take a look at splitting this PR into smaller units, however I found the breaking changes tended to depend on another (ex. the "automatic fix" feature is built off of the new configuration structure) making the task of splitting chunks a task of writing features twice. Because of this issue, I'm going to leave this PR as a monolith for now--if there is a particular chunk that you would like extracted, however, let me know and I'd be happy to look into it.

Once again: thanks for taking your time to review this. Please let me know if there is anything else I can do to make the process easier.

@caniszczyk
Copy link
Member

@prototypicalpro I've started to review the changes, this would go a bit faster if I could ask you some questions live, do you want to grab time with me over the next week? https://calendly.com/caniszczyk

@prototypicalpro
Copy link
Collaborator Author

@caniszczyk Sure! I scheduled a meeting for Sept. 10th 8:30am PST. I'd also be open to meeting longer if needed.

@caniszczyk
Copy link
Member

Just to update everyone, @prototypicalpro and I met and reviewed the changes.

Internally at the LF we have tested this on some of our projects and nothing has broken so far. The old repolinter format is supported and the test cases are in default-legacy.json so if we need to improve them, we can do that. I am going to propose a draft v1.0 RC1 with these changes for 4 weeks to give people time to give feedback and test before we will declare a v1.0

We will also add @prototypicalpro a maintainer to the project moving forward since he volunteered to maintain things moving forward and also work on contributing a github action down the line.

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.

Enhance the docs/ site with details on Repo Linter Make everything async
3 participants