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 things into different files, to optimize future work #85

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

Nercury
Copy link
Collaborator

@Nercury Nercury commented Mar 4, 2025

Not much change besides the simple divide and conquer.

@Nercury Nercury requested a review from MarijnS95 March 4, 2025 14:52
@Nercury
Copy link
Collaborator Author

Nercury commented Mar 4, 2025

@dextero I want this to land before #84

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Nice idea! rustfmt still needs to be ran and I think that should be(come) a requirement in the CI?

EDIT: I didn't really vet the changes at all but trust you moved it completely - CI is happy so the imports / module references should all be good.

@Nercury
Copy link
Collaborator Author

Nercury commented Mar 4, 2025

Nice idea! rustfmt still needs to be ran and I think that should be(come) a requirement in the CI?

EDIT: I didn't really vet the changes at all but trust you moved it completely - CI is happy so the imports / module references should all be good.

I have added basic rustfmt config. Other PRs can take care of that if that's important.
I tried to leave the public facing api exactly the same. Later the better names could be found for modules if necessary, but the single file was getting unwieldy.

@Nercury Nercury merged commit 151aefc into master Mar 4, 2025
24 checks passed
@MarijnS95
Copy link
Member

I have added basic rustfmt config. Other PRs can take care of that if that's important.

Oh I didn't need a rustfmt.toml config; if anything the edition set now should by default be fetched from Cargo.toml?

I meant running it in CI to reject changes if they don't adhere to the (default!) formatting standard, instead of causing unnecessary reformatting churn later on.

@Nercury
Copy link
Collaborator Author

Nercury commented Mar 4, 2025

I have added basic rustfmt config. Other PRs can take care of that if that's important.

Oh I didn't need a rustfmt.toml config; if anything the edition set now should by default be fetched from Cargo.toml?

I meant running it in CI to reject changes if they don't adhere to the (default!) formatting standard, instead of causing unnecessary reformatting churn later on.

It's ok I formatted with rustfmt now :)

@Nercury Nercury deleted the refactor-structure branch March 4, 2025 15:38
@MarijnS95
Copy link
Member

Oh yeah I trust that you formatted it now, just thinking it'd be nice to validate this in CI going forward for future contributions and pushes, as done in #86 :)

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