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

Restructuring nufmt #48

Open
IogaMaster opened this issue Sep 27, 2023 · 13 comments · May be fixed by #49
Open

Restructuring nufmt #48

IogaMaster opened this issue Sep 27, 2023 · 13 comments · May be fixed by #49

Comments

@IogaMaster
Copy link
Contributor

IogaMaster commented Sep 27, 2023

Let's structure nufmt like this project.
https://github.com/kamadorueda/alejandra

Mainly using workspaces

@IogaMaster IogaMaster changed the title Rework Restructure Sep 28, 2023
@IogaMaster IogaMaster changed the title Restructure Restructuring nufmt Sep 28, 2023
@IogaMaster IogaMaster linked a pull request Sep 28, 2023 that will close this issue
@sholderbach
Copy link
Member

As someone involved with the CI and crate setup of https://github.com/nushell/nushell I will say: Folks tend to have misconceptions how things behave in workspaces with different cargo commands. If there is no technical need for separate crates, my take, let's not shuffle things around just to have things familiar for one or two folks.

@IogaMaster
Copy link
Contributor Author

It's less about familiarity and more about proper separation of concerns.
I do want to hear @AucaCoyan @fdncred @amtoine talk about what they think

@AucaCoyan
Copy link
Contributor

Sounds good, but I don't see the improvement. What's the reason for changing the structure?
I'm still open to change it, but is it better to be different than the other nushell's org repos?
I'm missing something

@fdncred
Copy link
Collaborator

fdncred commented Sep 28, 2023

I thought you deleted too much, docs, benches, git workflows, etc. I can live with the rest but I defer to @AucaCoyan and @amtoine. They have been leading the charge here. I also wouldn't minimize @sholderbach's vast knowledge.

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Sep 28, 2023

In my pr there are two crates.

Nufmt
Nufmt_cli

The former is the library it does all the formatting
The latter is a wrapper around the lib.
This structure keeps them separate.

I can structure it more like the core nushell project if it's better.

@IogaMaster
Copy link
Contributor Author

@fdncred i was going to add those back just didn't have the time yesterday

@amtoine amtoine linked a pull request Sep 28, 2023 that will close this issue
@amtoine
Copy link
Member

amtoine commented Sep 28, 2023

i think the biggest issue we all have here is understanding the value of such big changes 🤔

i was going to add those back just didn't have the time yesterday

let's assume these files are put back, no worries, it's true that the PR is still a DRAFT 😉

i still do not get why we need such changes 😕

  • we already have a lib and a bin, why do we need two real crates?
  • for now, i only understand "here is a project written in Rust with a totally different structure, let's use it", but i'm not really willing to land such a big PR on that statement only... 😢

@IogaMaster
if you can put back the files that shouldn't have been removed and justify the refactor a bit more so that there is value in both reviewing and landing the changes, i would be more than happy and i'll let @AucaCoyan decide if he likes them 😌
i might have been the first one to be interested by this, but he has been by far the number one contributor to Nufmt 🙏

Important
@IogaMaster
all this is not at all to undermine your work, motivation and efforts, quite the opposite! we're just looking for a direction for all of us to go together, which is the formatter 😉
and btw, these other PRs of yours were really good 👍 😋 (#43, #45 and #47)

again, thanks for finding interest in Nushell and willing to help in developing its tooling 🙏

@IogaMaster
Copy link
Contributor Author

i still do not get why we need such changes

Complete separation of the two functions of this project would allow the lib to be consumed (and published on crates io) separate from the bin.
It makes tracking changes easier, if someone were to make a change to the cli that needs a change in the lib, those could be separate PR's. Meaning reverting changes would be easier.

In general decoupling makes the project scale better, and makes it easier to understand when contributing.

all this is not at all to undermine your work,

I know, I understand making a big change like this needs a lot of work and justification. 😀
Thanks for your help! 👌

@IogaMaster
Copy link
Contributor Author

If we continue with the rewrite I will draft a plan for how to proceed.

@AucaCoyan
Copy link
Contributor

Hi! on the #49 is a really good theoretical justification, in which I agree!, I quote it here:

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.

The question that rises over me now is: I think it's a good proposal for a project, but is it for this medium size nufmt? My points against it are:
if we look at the roadmap on #11 and the list of open issues:

  • nufmt binary won't grow much, probably it will go to 1.0 as it is today
  • All pending changes are going to be in the library (lib.rs and the mods)

and 2 other opinionated reasons:

  • I advocate for a philosophy of simplicity, even if a new feature or organization is faster, or more appropiate in the rust way, sometimes I chose not to. Why? because when you keep things simple, is (in a time perspective) shorter to understand. For example, If we choose to go with Traits, people new to rust with zero or poor knowledge won't be able to contribute.
    I find workspaces a bit redundant if you don't have 3 or more crates, especially if one (the binary) does nothing else than call the libray. Un-coupling them feels weird more than right, from my perspective of view.
  • As a nushell org, to my knowledge today are a couple of satellite projects related with formatting and parsing that are not met and agreed upon. We have more than one lsp (vs code, nuls) other tools like tree-sitter, a repo called grammar, among others I don't remember now or are deeply buried in history of the organization & collaborators repos

By all means, I don't want to undermine you! I find good to challenge the status quo. I only don't buy it yet.
Am I wrong? what do you think?

@IogaMaster
Copy link
Contributor Author

I can see where you are coming from on the simplicity front, but I think in the long term it would be better. What is on the roadmap is just for now.

As a nushell org, to my knowledge today are a couple of satellite projects related with formatting and parsing that are not met and agreed upon. We have more than one lsp (vs code, nuls) other tools like tree-sitter, a repo called grammar, among others I don't remember now or are deeply buried in history of the organization & collaborators repos

Maybe I need to make an RFC to talk about building a unified language and style spec.

@amtoine
Copy link
Member

amtoine commented Sep 29, 2023

an RFC might be good but sounds out of the scope of this issue and the associated #49 😌

@IogaMaster
Copy link
Contributor Author

Yeah, just the start of this process. 😉

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 a pull request may close this issue.

5 participants