Skip to content

Restructured codebase #104

Closed
RPG-Alex wants to merge 6 commits into
brendanzab:masterfrom
RPG-Alex:master
Closed

Restructured codebase #104
RPG-Alex wants to merge 6 commits into
brendanzab:masterfrom
RPG-Alex:master

Conversation

@RPG-Alex
Copy link
Copy Markdown

Previously the codebase was largle relegated to large module files, e.g. abs_diff_eq.rs, I have broken these up into modules like:

abs_diff_eq/
├── collections.rs
├── core_impls.rs
├── external.rs
├── mod.rs
├── primitives.rs
├── trait_def.rs
└── tuples.rs

There hasn't been any other major changes, it was mostly moving the code into the new home. This PR only covers approx and not the derive implementation. Will do derive next if this is acceptable.

@jonaspleyer
Copy link
Copy Markdown
Collaborator

To be completely honest. I do not really se the value in this PR. Furthermore you have introduced new tests in commit f3168d5 which is a change to the project and conflicts the description of your PR. To me, any length below 2000 LOC per file is absolutely acceptable. If you wish to add the tests, please open a new PR and explain your reasoning behind the tests.

@RPG-Alex
Copy link
Copy Markdown
Author

Thanks for the feedback.

I want to clarify the purpose of this PR. The goal is not to change the behavior of the crate, but to improve the structure of the existing implementation by breaking large module files into smaller, responsibility based modules.

Instead of keeping most of the AbsDiffEq implementation in one large file, this PR organizes it as:

abs_diff_eq/
├── collections.rs
├── core_impls.rs
├── external.rs
├── mod.rs
├── primitives.rs
├── trait_def.rs
└── tuples.rs

The value of this is mainly maintainability: related implementations are easier to find, future changes are easier to review, and the codebase becomes easier to extend without continuing to add to already-large files. I don't use LOC for determining how to modularize, and if that is a consideration here I understand.

Regarding commit f3168d5, those were tests that I didn't see present and wanted to increase test coverage. Given the tests do not impact the runtime I didn't see the need to differentiate another PR solely for testing.

@jonaspleyer
Copy link
Copy Markdown
Collaborator

I don't see the value in fragmenting the code into multiple files of <100 LOC. To me this is a regression. Again, if you want to add the tests, simply open a new PR.

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.

2 participants