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

Reorganize linters and fixers #22650

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Reorganize linters and fixers #22650

merged 4 commits into from
Mar 28, 2024

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Mar 18, 2024

This PR reorganizes the linters and fixers structure, as discussed in the last BCD meeting. This places all of the fixer scripts under the same folder that the linters are, and renames the folder from test/ to lint/.

This ensures both better organization and prepares for a potential linter overhaul in the future.

@github-actions github-actions bot added bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes scripts 📜 Issues or pull requests regarding the scripts in scripts/. labels Mar 18, 2024
@github-actions github-actions bot added infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project dependencies ⛓️ Pull requests that update a dependency package or file. labels Mar 18, 2024
@github-actions github-actions bot removed the dependencies ⛓️ Pull requests that update a dependency package or file. label Mar 28, 2024
@queengooborg queengooborg merged commit 4d6bfff into main Mar 28, 2024
9 checks passed
@queengooborg queengooborg deleted the test/move-files branch March 28, 2024 12:22
@Elchi3
Copy link
Member

Elchi3 commented Mar 28, 2024

Why was this merged without review?

@queengooborg
Copy link
Collaborator Author

queengooborg commented Mar 28, 2024

We had agreed to do this during the BCD call on March 12th. I didn't think this would require review since it is solely reorganizing internal infrastructure.

I see this had created merge conflicts for one of your PRs, though. Apologies for that -- happy to fix that right away!

@Elchi3
Copy link
Member

Elchi3 commented Mar 28, 2024

It would have been useful to still review in more details. In meetings we usually only assess if a PR makes sense generally, ie. if we agree with the notion of it and/or general direction.

These file names seem not ideal:

test-browsers-data.ts
test-browsers-presence.test.ts

Why not omit the first "test"?
browsers-data.ts
browsers-presence.test.ts

@queengooborg
Copy link
Collaborator Author

That's a fair point. I figured that this particular change was straightforward enough that it wouldn't need a thorough technical review.

The filenames for the linters is something that I've also been wanting to update to remove the redundant test- prefix, but I figured that would be better to do in another PR since that had a stronger likelihood of creating merge conflicts with other PRs. (Turns out that the open linter-related PRs all had merge conflicts anyways...!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project scripts 📜 Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants