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

Fix all non-bug lints. Switch to let statements. #56956

Closed
wants to merge 124 commits into from

Conversation

JohnHeitmann
Copy link
Contributor

@JohnHeitmann JohnHeitmann commented Dec 18, 2018

After these changes there is only one error report remaining from the linter, and that appears to be a real bug.

The first commit has all the non-trivial changes. The second is a mechanical s/var/let/.

cc: #51735

@JohnHeitmann
Copy link
Contributor Author

I'm embarrassed to admit I can't find where those unknown origin globals are coming from. If you can point out their source I'll update those docs.

@JohnHeitmann
Copy link
Contributor Author

Ah, right after submit I found it. Unique DOM ids are automatically added to window.

@Centril
Copy link
Contributor

Centril commented Dec 24, 2018

r? @QuietMisdreavus

@bors
Copy link
Contributor

bors commented Dec 26, 2018

☔ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 26, 2018
@Centril
Copy link
Contributor

Centril commented Dec 26, 2018

@JohnHeitmann Please avoid merge commits; we'd prefer rebasing instead. :)

@JohnHeitmann
Copy link
Contributor Author

Sorry about that. I force-pushed a clean rebase. I kept the two separate commits to keep the more interesting changes apart from the search and replace change.

@bors
Copy link
Contributor

bors commented Dec 29, 2018

☔ The latest upstream changes (presumably #57006) made this pull request unmergeable. Please resolve the merge conflicts.

This change fixes all non-bug lints. A few lint rules are tweaked, and a few code lines are tweaked (almost all dead code elimination).

Also, update the rustdoc-js tester in anticipation of let/const statements.
@QuietMisdreavus
Copy link
Member

cc @GuillaumeGomez for changes to JS

Do we run eslint in CI? Is it particularly time-intensive? @rust-lang/infra Would it be possible to slot it into one of our existing builders?

@kennytm
Copy link
Member

kennytm commented Jan 2, 2019

Note: https://caniuse.com/#search=let


@QuietMisdreavus

Do we run eslint in CI?

I can't find anything about eslint in this repo 🙃. If we could make it runnable via ./x.py test ..., we could add this to mingw-check or x86_64-unknown-linux-gnu (depending on whether a bootstrap is needed).

@JohnHeitmann
Copy link
Contributor Author

While it runs fast, eslint is a node app. Another thread made it sound like node was a challenging dependency to impose upon some distros, so I assumed this would be a best-effort tool used by humans.

If node is ok in the build after all I could take a look at adding this to CI.

@kennytm
Copy link
Member

kennytm commented Jan 3, 2019

We already depend on nodejs to run the src/test/rustdoc-js tests (the test is skipped if nodejs is missing).

@JohnHeitmann
Copy link
Contributor Author

This is my plan for moving this forward. Please steer me if this looks like the wrong direction:

  1. Implement a build.config.eslint setting in https://github.com/rust-lang/rust/blob/master/src/bootstrap/test.rs
  2. Run eslint in the rustdoc-js test target
  3. Update the test travis config to install eslint via npm like clippy does with remark: https://github.com/rust-lang/rust-clippy/blob/39bd84494f4a9b40f2e3b38416a91d52d5c4738b/.travis.yml#L29
  4. Somehow test the travis build. Do I just need to set up my own travis instance and it'll just work? Let me know if there are mines here.
  5. Fix the build blockers after we see the build is properly blocked (there's one unfixed bug the lint catches).

@QuietMisdreavus
Copy link
Member

Somehow test the travis build. Do I just need to set up my own travis instance and it'll just work? Let me know if there are mines here.

If you need to run a specific travis instance on your PR without trying to merge it, you can edit the travis.yml file to run a specific builder on pull requests as well instead of just the "auto" branch. (I'd recommend also renaming the PR here to add "WIP" to it so bors doesn't accidentally merge the travis configuration change.)

nikomatsakis and others added 27 commits January 5, 2019 00:36
Remove the leak-check and its associated machinery. Replace with
making the solver aware of universes.
Amazingly, this scenario was not tested for trait matching.
they are subsumed by `hr-subtype/hr-subtype.rs` and other tests
In NLL, ReVid is all there is, but I might want to repurpose.
In particular, when we want to indicate that there is a connection
between the self type and the other types.
Still not great, but good enough to land this PR.
i_e is not defined, so in some cases this block cannot work. Furthermore, the toggle-wrapper is added after this block runs, so the hasClass(toggle-wrapper) check never seems to run. This appears to be dead code.
@JohnHeitmann
Copy link
Contributor Author

JohnHeitmann commented Jan 5, 2019

I just did something horribly wrong in my git workflow on this branch and I'm not confident I can back it out without tagging more unrelated issues. Closing this out to start from a fresh branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.