Skip to content

Fix eslint warnings and add eslint check in CI #2554

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

Merged
merged 4 commits into from
Mar 23, 2025

Conversation

GuillaumeGomez
Copy link
Member

First commit comes from #2552.

This PR fixed all eslint warnings and added it as a check in the CI. That is a first step in improving the JS quality.

cc @notriddle

@GuillaumeGomez GuillaumeGomez requested a review from ehuss February 21, 2025 22:14
@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Feb 21, 2025
@GuillaumeGomez GuillaumeGomez force-pushed the eslint branch 2 times, most recently from a87b0c6 to 7f4f189 Compare March 10, 2025 11:18
@GuillaumeGomez
Copy link
Member Author

Rebased and fixed merge conflict.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Can you add documentation to https://github.com/rust-lang/mdBook/blob/master/CONTRIBUTING.md with links to what this is, how to install it, and some information on how to run and use it?

@@ -64,7 +64,6 @@ pub fn create_files(
}

if search_config.copy_js {
static_files.add_builtin("searchindex.json", index.as_bytes());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this line changing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it's from another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! From #2552 to be more exact.

.eslintrc.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say how all the rules for this were created? Why are the styles different from the defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the rules we use in rustdoc. "defaults" change from time to time, so with this set of rule, at least it's fixed.

if let Some(version) = line.strip_prefix("BROWSER_UI_TEST_VERSION:") {
return version.trim().replace('\'', "");
let content = read_to_string("package.json").expect("failed to read `package.json`");
let json = json::parse(&content).expect("invalid JSON");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this use serde_json since it is already in the dependency tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@GuillaumeGomez
Copy link
Member Author

Can you add documentation to https://github.com/rust-lang/mdBook/blob/master/CONTRIBUTING.md with links to what this is, how to install it, and some information on how to run and use it?

Sure!

@GuillaumeGomez
Copy link
Member Author

Applied suggestions.

@GuillaumeGomez
Copy link
Member Author

Rebased.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss added this pull request to the merge queue Mar 23, 2025
Merged via the queue into rust-lang:master with commit b47d1cf Mar 23, 2025
12 checks passed
@GuillaumeGomez GuillaumeGomez deleted the eslint branch March 23, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants