nushell / nufmt

MIT License
74 stars 10 forks source link

Restructuring nufmt #48

Open IogaMaster opened 1 year ago

IogaMaster commented 1 year ago

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

Mainly using workspaces

sholderbach commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

amtoine commented 1 year ago

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

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 :wink:

i still do not get why we need such changes :confused:

@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 :relieved: i might have been the first one to be interested by this, but he has been by far the number one contributor to Nufmt :pray:

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 :wink: and btw, these other PRs of yours were really good :+1: :yum: (#43, #45 and #47)

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

IogaMaster commented 1 year ago

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. :grinning: Thanks for your help! :ok_hand:

IogaMaster commented 1 year ago

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

AucaCoyan commented 1 year ago

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:

and 2 other opinionated reasons:

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 commented 1 year ago

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 commented 1 year ago

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

IogaMaster commented 1 year ago

Yeah, just the start of this process. :wink: