-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
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:
|
d207504
to
f6b88af
Compare
f6b88af
to
d7c2df0
Compare
There was a problem hiding this 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 🥳
There was a problem hiding this comment.
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 😋
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😌
There was a problem hiding this comment.
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 😋
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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? 😇
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😉
I would close this as this PR didn't see the light in a while. |
ya, it has been a while |
this will close #48
changelog
pull_request.yml
andmain.yml
intocheck_rust.yml
benches/
which were very non-maturecrates/nufmt/src/lib.rs
nufmt
andnufmt_cli
insidecrates/
Justification
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.
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.
Easier Onboarding: When new team members join nufmt, a decoupled codebase makes it easier for them to get up to speed.
Maintainability: A more understandable codebase is inherently more maintainable. Developers can make changes and improvements more confidently, reducing the risk of introducing new issues.
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.