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

Split up files with // ignore-tidy-filelength #60302

Open
8 tasks
varkor opened this issue Apr 26, 2019 · 49 comments
Open
8 tasks

Split up files with // ignore-tidy-filelength #60302

varkor opened this issue Apr 26, 2019 · 49 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@varkor
Copy link
Member

varkor commented Apr 26, 2019

These files are over 3,000 lines, which is not ideal for navigating or comprehending. If we can, it would be good to split these up. See also #60015, which is a specific issue of this problem.

  • rustc_parse/src/parser/expr.rs
  • rustc_borrowck/src/diagnostics/conflict_errors.rs
  • rustc_trait_selection/src/error_reporting/traits/suggestions.rs
  • rustc_hir_typeck/src/method/suggest.rs
  • rustc_resolve/src/late.rs
  • rustc_resolve/src/late/diagnostics.rs
  • src/librustdoc/html/static/js/search.js
  • src/tools/compiletest/src/runtest.rs tracked separatedly by Splitting up compiletest::runtest #89475
@varkor varkor added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Apr 26, 2019
@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 26, 2019
@petrochenkov
Copy link
Contributor

#61097 is going to help with some of the files.

@jonas-schievink jonas-schievink added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 15, 2020
@olamiko
Copy link

olamiko commented Apr 28, 2020

Which files @varkor? The files in #60015 are done

@varkor
Copy link
Member Author

varkor commented Apr 28, 2020

I count 17 different files that still have this comment.

@alex-griffiths
Copy link

Hey, I'm not sure if anyone is working on this, but, this looks like it might be a good place to get started contributing.

I am quite new to the language, but I'd like to get involved. Is this something as simple as pulling out structs and other related blocks of code into their own files and then including them in the original?

@varkor
Copy link
Member Author

varkor commented Jun 3, 2020

@alex-griffiths: yes, it should be as simple as figuring out some sensible way to split up large files into related types and implementations!

@alex-griffiths
Copy link

alex-griffiths commented Jun 3, 2020

@varkor Alright, mind if I take a crack at this?

@varkor
Copy link
Member Author

varkor commented Jun 3, 2020

@alex-griffiths: yes, go ahead! You can claim the issue with:

@rustbot claim

@alex-griffiths
Copy link

@rustbot claim

@rustbot rustbot self-assigned this Jun 3, 2020
@alex-griffiths
Copy link

alex-griffiths commented Jul 1, 2020

@varkor I'm finding that a lot of the files that include the // ignore-tidy-filelength comment are files that include lots of the documentation.

I'm still kind of new to the language, but for example in src/libstd/fs.rs there are 6 structs where the struct definition itself is a single line, but with varying amounts of docs. Is this the kind of thing that is able to be extracted into it's own file which is then included as a module in the fs.rs file?

@varkor
Copy link
Member Author

varkor commented Jul 1, 2020

@alex-griffiths: that's an interesting observation. I think in these cases, it's not so important to split up the files, because the length doesn't pose an issue for comprehension. Anyone editing the file is going to be interested in one or two particular functions, which are well-isolated within the file, and wouldn't benefit from being in their own file. In some sense, files like these are "false positives", though I don't think we could automatically exclude them. The lint is really more for files with large functions, and interdependency that we want to make easier to navigate. Thanks for pointing this out!

@alex-griffiths
Copy link

I'll start working through all the files that have the comment in them, and I'll compile a list of files that appear to mostly documentation, and then for the rest, I'll start trying to improve the navigability of them.

@danii
Copy link
Contributor

danii commented Sep 2, 2020

Would like to help with this, too. Is there a possibility to share an issue?
Specifically, I have my eyes on core::num.

@alex-griffiths
Copy link

Yeah I'm happy with this @varkor.

Perhaps we could do something similar to how #75080 is managing the multiple people doing for that issue?

@jyn514
Copy link
Member

jyn514 commented Sep 2, 2020

Seems like a good idea. You need

  • a list of the files to be split up
  • instructions for how to split them up (more detailed is better, but simple is fine as long as you update it along the way)
  • someone to keep the tracking issue up to date as people claim files. Maybe in this case that would be @varkor?

@danii
Copy link
Contributor

danii commented Sep 3, 2020

Oh sweet, thanks! This'll be my first contributions to the rust standard library. 😄
I personally don't think we'll need instructions on splitting the files up as long as everyone's respectful. Although, it is @alex-griffiths decision.
If no one minds I'm gonna list out the files that need truncating.

@danii
Copy link
Contributor

danii commented Sep 3, 2020

This is the list of all the files that need some splitage. ✅ #00000 means a split is done, 🔲 #00000 means it's in progress, and just 🔲 means it's free for the taking.

Out Of Scope

These files are very long, but they should be handled in a different manner.

@alex-griffiths
Copy link

I believe changes to rustc_resolve need to be handled in a different manner.
#75701 (comment)

@tesuji
Copy link
Contributor

tesuji commented Sep 4, 2020

I am doing core::slice splitting (#76311 )

@tesuji
Copy link
Contributor

tesuji commented Sep 4, 2020

Hi @alex-griffiths, Would you mind if I take care of core::str part ?

@jyn514
Copy link
Member

jyn514 commented Sep 4, 2020

@lzutao maybe do core::num first since alex is in the middle of splitting core::str

@tesuji
Copy link
Contributor

tesuji commented Sep 4, 2020

Yeah, he also did alloc/vec.rs in #75701 . But he seems having trouble in splitting core/str.rs.
I would like to help him a hand.

Edit: It's up as a draft PR #76325

JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 6, 2021
…-comment, r=Mark-Simulacrum

Ignore comments in tidy-filelength

Ref rust-lang#60302 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 6, 2021
…omment, r=Mark-Simulacrum

Ignore comments in tidy-filelength

Ref rust-lang#60302 (comment)
@pierwill
Copy link
Member

pierwill commented Oct 26, 2021

Looks like there are three left in compiler/:

@dtolnay
Copy link
Member

dtolnay commented Jan 27, 2022

@rustbot release-assignment

@rustbot rustbot removed their assignment Jan 27, 2022
@yerke

This comment was marked as outdated.

@yerke

This comment was marked as outdated.

@workingjubilee
Copy link
Member

Further cleanup should focus on the files in rustc_resolve/src as the motivation for improving navigation in does not apply to the tests in question in rustc_apfloat, in effect.

@jmj0502
Copy link
Contributor

jmj0502 commented Jul 19, 2022

Hey! I would like to start contributing to the project and this seems like a good starting point. Is the compiletest::runtest file still available?

@workingjubilee
Copy link
Member

Yes, go ahead.

@Sam-Buckley
Copy link

I'd like to start contributing, and was directed here as a good starting point. What's still outstanding in this issue?
@rustbot claim

@workingjubilee
Copy link
Member

@Sam-Buckley I updated the top of this issue with the current list: #60302 (comment)

@jieyouxu
Copy link
Member

Updated issue description to not include compiletest::runtest, which is tracked separately in #89475.

@Sam-Buckley
Copy link

Sorry to waste everyone's time, family matters came up and I'll have to stop for a while

@Sam-Buckley
Copy link

@rustbot release-assignment

@ranger-ross
Copy link
Contributor

Hey there, I'll take a crack at cleaning some of these large files up.

@rustbot claim

@NoobProgrammer31
Copy link

@rustbot claim

assign this to me i will work upon it

@NoobProgrammer31
Copy link

can i start working on mod.rs first in first file ( i am new to it )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests