-
Notifications
You must be signed in to change notification settings - Fork 74
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
Conversation
fix: get jsdoc to run
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. |
@prototypicalpro no worries it's a small change so hopefully not too bad |
I've merged the |
Thanks Noah, it's going to take some time for us to review this change. I'm
going to spend some time this week and reach out to Trevor to get at last
another set of eyes on this, I also want to over communicate with current
adopters about the upcoming potential changes.
Ideally we would have appreciated a smaller stream of changes we could
review versus one giant PR. If you think there is a way to do that in a
sensible fashion, we'd prefer that but understand that it may be hard to
break out changes in logical chunks.
Thanks for pushing us to improve this tool though :)
…On Mon, Aug 31, 2020 at 1:10 PM Noah Koontz ***@***.***> wrote:
I've merged the contributor-count axiom in, and this PR should be good to
go.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#174 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAPSIPAH7OGSM6ISDLORM3SDPRQ5ANCNFSM4QOSRZ7A>
.
--
Cheers,
Chris Aniszczyk
http://aniszczyk.org
+1 512 961 6719
|
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. |
@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 |
@caniszczyk Sure! I scheduled a meeting for Sept. 10th 8:30am PST. I'd also be open to meeting longer if needed. |
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. |
Motivation
This PR addresses notes mentioned in #173.
Proposed Changes
Pulling from the newrelic-forks repolinter changelog:
Breaking
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.lint
function:lint
, allowing the developer to suppress or modify the output as needed. This change is reflected in the new CLI implementation.LintResult
) has been completely restructured.async
was added to the interface.lint
's return value.targetdir/otherdir/repolinter.json
would trigger another lint ofotherdir
) has been removed for now. I'd be open to adding this functionality back before this PR is merged.Features
--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.runRuleset
,determineTargets
,validateConfig
, andparseConfig
.Fixes
fs.promises
, which increased performance quite a bit.Notes
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!