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

Consolidate descriptions linter functions into one #4872

Merged
merged 27 commits into from
May 13, 2022

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Sep 23, 2019

Now that the descriptions linter has been merged (yay!), I'm opening up this PR to consolidate the code a little bit. Essentially, this PR consolidates the four functions testing for each type of description into a single function, which calls out to another function that performs both the check and the logger output based upon a set of arguments. This is to help keep the number of code lines to a minimum, and reduce the number of iterations through data the linter must perform.

I'm open to suggestions or revisions!

@ghost ghost added the linter Issues or pull requests regarding the tests / linter of the JSON files. label Sep 23, 2019
queengooborg added a commit to queengooborg/browser-compat-data that referenced this pull request Oct 25, 2019
Elchi3 pushed a commit that referenced this pull request Oct 29, 2019
* Standardize styling for linters

* Fix overuse of bolding

* Fix indentation of errors

* Undo changes to description tests (conflicts with #4872)
@queengooborg queengooborg mentioned this pull request Mar 27, 2020
23 tasks
@queengooborg queengooborg requested a review from ddbeck as a code owner October 8, 2020 22:18
@queengooborg queengooborg removed the request for review from Elchi3 October 8, 2020 22:18
@ddbeck ddbeck removed their request for review December 17, 2020 16:17
Base automatically changed from master to main March 24, 2021 12:53
@ddbeck
Copy link
Collaborator

ddbeck commented Nov 26, 2021

Hi @queengooborg, Florian and I talked about this in our most recent 1:1 (we're trying to get back in the habit of unblocking old PRs). It's not that we don't want to see the speed up here, but we're skittish about making changes to the linter, especially multiple changes at once (in here we've got style changes, a structural change, and what looks to be some jsdoc changes—it's not clear they're all strictly dependent on each other).

I don't think we can merge this as is, so I propose closing this and approaching these things from a different angle.

Looking forward, we've got three suggestions:

  • If you've got a change to the existing scripts, linters, etc., then they should be as small as possible. More PRs that do as little as possible is preferable. Counter-intuitive, but true.
  • If an existing linting needs to change, then it should be accompanied by some tests (with refactoring as needed to make something testable—see Fix preview consistency check #12673 for an example).
  • If there's new linting to be done, then it should be implemented as a standalone test (for now, as in spec-urls.test.js) or as part of the new linting architecture (an issue for which I still owe you, I think).

Let me know if there's any questions about this. Thank you!

@ddbeck ddbeck closed this Nov 29, 2021
@queengooborg
Copy link
Contributor Author

I just saw this PR was closed. I can understand the desire to keep changesets as small as possible, but to be honest, I don't see much room for stripping changes out. There are a couple of things I could split into other PRs like the JSDoc and formatting changes to the main entry point, but the rest of the PR contains changes that all go together.

The problem I'm trying to solve with this PR is that right now, we have repeated lines of code for the same operation, causing us to iterate through the data multiple times. What I'm attempting to do here is instead simplify this by introducing one function to iterate through the data and determine the parameters needed, which is then passed to another function that performs the check and logs any discrepancies. While adding these new functions and replacing the old, I'm also describing them in JSDoc to give a more complete PR so we don't have to bother following up later.

Basically, all of the changes between lines 9 and 101, along with lines 110 through 125 (all on the "before" side) are all related and can't be cherry-picked, at least not in a sensible way. The changes on other lines were included because they're so small on their own, they don't really warrant a separate PR, and we're already here working on this file.

@queengooborg queengooborg reopened this May 4, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits.

test/linter/test-descriptions.js Outdated Show resolved Hide resolved
test/linter/test-descriptions.js Outdated Show resolved Hide resolved
test/linter/test-descriptions.js Outdated Show resolved Hide resolved
queengooborg and others added 2 commits May 12, 2022 12:44
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

One last follow-up nit, but still looking good, of course.

test/linter/test-descriptions.js Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has merge conflicts that must be resolved before we can merge this.

@queengooborg queengooborg enabled auto-merge (squash) May 13, 2022 20:38
@queengooborg queengooborg merged commit a154214 into mdn:main May 13, 2022
@queengooborg queengooborg deleted the linter/descriptions-update branch May 13, 2022 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter Issues or pull requests regarding the tests / linter of the JSON files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants