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.

Sign up for GitHub

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

Jump to bottom

Expand lint scripts #25251

Merged
merged 10 commits into from
Jan 12, 2023
Merged

Expand lint scripts #25251

merged 10 commits into from
Jan 12, 2023

Conversation

LeviPesin
Copy link
Contributor

Related issue: -

Description

Expand lint test to include tests for addons, docs, and other directories. This PR also changes main lint script so these tests are ran by npm run lint (and therefore the lint test in the actions).

@LeviPesin LeviPesin marked this pull request as draft January 6, 2023 16:54
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 6, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@LeviPesin
Copy link
Contributor Author

CodeQL alerts on this PR because linting changes in some files trigger it (CodeQL thoughts that issues were both removed and added).

@LeviPesin LeviPesin marked this pull request as ready for review January 6, 2023 17:56
@LeviPesin
Copy link
Contributor Author

The number of lines changed is scary 😅

@LeviPesin
Copy link
Contributor Author

@mrdoob @Mugen87 What do you think about this PR? I think it is good to have lint test to warn if incorrect linting appears in addons (docs, etc).

@gkjohnson
Copy link
Collaborator

I recommend making smaller PRs with a smaller sets of files linted per PR so the change set is more approachable. And then a separate PR with an automated lint process added.

The /manual directory has never matched the project code style since it was developed in another source - so I expect that to require a large set of changes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 11, 2023 •
edited

Agreed! It's always hard to review such large PRs. It's easy to overlook something so smaller change sets are preferable.

I would exclude linting manual for now. It's not only the code style which is somewhat out of line comparing to the examples.

@Mugen87 Mugen87 closed this Jan 11, 2023
@LeviPesin
Copy link
Contributor Author

Reverted the lint changes to manual. Can this PR be reopen, please?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 11, 2023

I think it would be also better to revert the changes to package.json. That should also be discussed in a separate PR.

@LeviPesin
Copy link
Contributor Author

Done.

@Mugen87 Mugen87 reopened this Jan 11, 2023
Mugen87
Mugen87 reviewed Jan 11, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@Mugen87 Mugen87 added this to the r149 milestone Jan 11, 2023
@LeviPesin LeviPesin changed the title Expand lint test Expand lint scripts Jan 11, 2023
@Mugen87 Mugen87 merged commit a583db2 into mrdoob:dev Jan 12, 2023
@LeviPesin LeviPesin deleted the patch-4 branch January 12, 2023 08:47
@LeviPesin LeviPesin mentioned this pull request Jan 12, 2023
Merged
@LeviPesin LeviPesin mentioned this pull request Jan 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mugen87 Mugen87 Mugen87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r149
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LeviPesin @gkjohnson @Mugen87