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

Add tips/hints to version test #5605

Merged
merged 6 commits into from
May 13, 2022

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Jan 29, 2020

This PR introduces tips to the version linter for common mistakes contributors may make when creating a PR. This was inspired by a comment Daniel posted on a suggestion to rename "safari_ios" to "ios":

It seems reasonable to me that if you try to use a valid desktop version number for iOS, then the linter could remind you of the possibility that you're following the wrong versioning scheme.

Well, here you go, @ddbeck -- one fresh addition of code hot off the grill! 😄

@ghost ghost added the linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. label Jan 29, 2020
@ddbeck ddbeck self-requested a review February 17, 2020 14:07
@queengooborg queengooborg mentioned this pull request Mar 27, 2020
23 tasks
@ddbeck ddbeck removed their request for review September 8, 2020 14:12
Base automatically changed from master to main March 24, 2021 12:54
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 whitespace nit and one refactoring opportunity, but LGTM and could be merged as is.

test/linter/test-versions.js Outdated Show resolved Hide resolved
@@ -103,12 +111,24 @@ function checkVersions(supportData, relPath, logger) {
for (const statement of supportStatements) {
if (!isValidVersion(browser, statement.version_added)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to deduplicate these two blocks into a for ... of loop:

for (const property of ['version_added', 'version_removed']) {
  // ...
}

@@ -103,12 +111,24 @@ function checkVersions(supportData, relPath, logger) {
for (const statement of supportStatements) {
if (!isValidVersion(browser, statement.version_added)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to deduplicate these two blocks into a for ... of loop:

for (const property of ['version_added', 'version_removed']) {
  // ...
}

@github-actions
Copy link

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

@queengooborg queengooborg merged commit eb9becb into mdn:main May 13, 2022
@queengooborg queengooborg deleted the linter/versions-tips branch May 13, 2022 20:35
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.

2 participants