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 datastore.rs into models.rs and tests.rs #1656

Merged
merged 5 commits into from
Jul 31, 2023
Merged

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Jul 28, 2023

Prevent this file from attaining critical mass.

To get this to merge and retain git history, we will have to temporarily allow merge or rebase commits on this repository. Squashing the commits will cause the history to be lost.

The drawback to keeping the distinct commits is that this harms use of git bisect, these commits would have to be git bisect skip'd.

@inahga inahga requested a review from a team as a code owner July 28, 2023 21:11
@jbr
Copy link
Contributor

jbr commented Jul 28, 2023

External opinion: As long as you're moving stuff around, why not break the models file into a file per model and reexport from the models module? 1700 lines is still a really long file even after this change. One file per type makes it really easy to jump to exactly what you're looking for and keep type definitions, impls, and small associated types together

@inahga
Copy link
Contributor Author

inahga commented Jul 28, 2023

I'll have to defer to the rest of the team @divviup/committers for an opinion on that, since they have spent far more time in the codebase than I. Then maybe we can carry that forward into the style guide https://github.com/divviup/janus/blob/main/docs/DEVELOPMENT.md.

I myself get along decently with go-to-definition and go-to-reference, but that workflow has reached its limits at this large of a file, hence my immediate urge to break it down.

@tgeoghegan
Copy link
Contributor

tgeoghegan commented Jul 30, 2023

While models.rs is rather large (1,700+ lines), I think mandating one file per struct is an overcorrection in the other direction. I also think we should try to avoid imposing format or layout rules on a project unless they can be enforced by automation, because policing conformance with such rules is a poor use of engineer time. I say we go ahead with models.rs as it is in this PR.

@inahga inahga merged commit 13fceb6 into main Jul 31, 2023
7 checks passed
@inahga inahga deleted the inahga/file-size branch July 31, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants