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 lib and bin into crates and minor refactoring #49

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

IogaMaster
Copy link
Contributor

@IogaMaster IogaMaster commented Sep 28, 2023

this will close #48

changelog

  • merge workflows pull_request.yml and main.yml into check_rust.yml
  • remove the benches/ which were very non-mature
  • move tool functions to crates/nufmt/src/lib.rs
  • split code base into nufmt and nufmt_cli inside crates/

Justification

  1. Improved Readability: Decoupling the code into modular packages can enhance code readability. It allows developers to focus on one specific aspect of the code at a time, making it easier to understand.

  2. Reduced Cognitive Load: With decoupled packages, developers don't need to grasp the entire codebase in one go. They can delve into specific modules as needed, reducing the cognitive load and making it more accessible.

  3. Easier Onboarding: When new team members join nufmt, a decoupled codebase makes it easier for them to get up to speed.

  4. Maintainability: A more understandable codebase is inherently more maintainable. Developers can make changes and improvements more confidently, reducing the risk of introducing new issues.

  5. Long-term Viability: A codebase that is hard to understand can become a liability in the long run. By investing in a rewrite now, we can ensure the long-term viability and sustainability of nufmt.

@IogaMaster IogaMaster changed the title Rework (Draft) Restructure Sep 28, 2023
@IogaMaster
Copy link
Contributor Author

Alright all basic functionality is restored in this pr. Please test and let me know what else is in need of changing.

I still need to work on:

  • Setting the CI back up

@amtoine amtoine linked an issue Sep 28, 2023 that may be closed by this pull request
@IogaMaster IogaMaster force-pushed the restructure branch 2 times, most recently from d207504 to f6b88af Compare September 28, 2023 21:59
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

now that i have a justification for this PR, i think the changes are pretty great 😋

i have put the details of the changelog in the PR description above 👍

however, there are a few points in the review threads that need to be addressed before landing this PR 😉 😌

@AucaCoyan
if you can try the new nufmt on some files and run the tests and tell me that looks good, one the review threads are closed, then let's go with these changes 🥳

Copy link
Member

Choose a reason for hiding this comment

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

i would like this file to be resurrected 😋

Copy link
Contributor Author

@IogaMaster IogaMaster Sep 29, 2023

Choose a reason for hiding this comment

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

From a comment on the issue from Auca, he said there are a lot of different standards being worked on at the same time. Maybe we need an RFC to talk about making a unified language spec?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that's fair 👍

but for the time being, this is the only place we found for this file...
so, until we have a proper spec somewhere, i'd rather leave it there 😌

Copy link
Member

Choose a reason for hiding this comment

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

i would like this file to be resurrected 😋

Copy link
Member

Choose a reason for hiding this comment

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

this file should stay called toolkit.nu, it's not a test file, but rather a pure-Nushell Makefile alternative 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using that to test formatting and forgot to rename it. :p

Copy link
Member

Choose a reason for hiding this comment

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

no worries, as long as you rename it, that's fine 😉

}

#[cfg(test)]
mod test {
Copy link
Member

Choose a reason for hiding this comment

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

i do not see these tests anywhere now...

can you put them back in the nufmt crate? 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why tests arent running, I will investigate

Copy link
Member

Choose a reason for hiding this comment

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

well, if the tests are not in the source base anywhere, they won't run, right 😉

@amtoine amtoine changed the title (Draft) Restructure Split lib and bin into crates and minor refactoring Sep 29, 2023
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.

Restructuring nufmt
2 participants