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 puppet.rs into a module #48

Closed
wants to merge 2 commits into from

Conversation

orowith2os
Copy link
Contributor

Previously, everything regarding puppet was in puppet.rs. Now, it's split up into puppet/ so that it's easier to manage and search though.

The API should be the same, save for the enum modifications made.

The modifications to the enums are meant to better reflect their usage, and real-world examples. For example, instead of ViralLicense, it's SameLicense. Instead of Copyleft, it's Permissive.

@orowith2os
Copy link
Contributor Author

Didn't mean to include some changes here, but w/e.

@orowith2os
Copy link
Contributor Author

CI is failing due to tests that were failing before my changes.

@Speykious
Copy link
Member

Yeah the CI is way too complex for this project, I was wrong to take it from Jon Gjengset... x_x

I'm gonna completely change it once the renderless PR is merged.

@Speykious
Copy link
Member

We had a long discussion about this file split on Discord. We couldn't get to understand our differing opinions unfortunately, but in any case I'll sum up why I don't agree to this change:

  • In Rust codebases, the tendency is to organize files by behavior and not by type. It has several advantages which I'm listing below and are the reason puppet.rs is this way. It's not always the case as sometimes a struct will have a huge impl block, but that's the exception rather than the norm and it certainly doesn't apply here.
  • When someone new to the codebase comes in, every struct that a puppet uses is gonna be in puppet.rs, as opposed to having to jump into a bunch of files to construct the bigger picture in their head. This makes the code much more readable.
  • It is not easier to search through. Every decent IDE and code editor has features such as Go To Definition and Go To References, thanks to LSPs. If you want to search for a struct, use that. We shouldn't rely on files to search for structs, we should use files to organize code in general. It's not just structs that should be easy to search, behavior should be too, and we're definitely not having one file per function and per struct, so organizing in terms of behavior is imo the best compromise.
  • When adding new features, very often we'll have to implement the same kind of behavior on all kinds of related structs, and puppet.rs applies perfectly to this use-case. For example, a split that I would most definitely accept is putting all the Display impls in their own files.

So yeah, in short, I won't accept this PR. But if you just split the display impls into their own single file I will :)

@Speykious Speykious closed this Jun 30, 2023
@orowith2os
Copy link
Contributor Author

Maybe reopen this so i can just push my changes rather than opening another PR.

@Speykious Speykious reopened this Jun 30, 2023
@Speykious
Copy link
Member

Hm ok

@orowith2os orowith2os marked this pull request as draft June 30, 2023 09:27
Signed-off-by: Dallas Strouse <dastrouses@gmail.com>
@orowith2os
Copy link
Contributor Author

Need input on if I should also do a try_from.rs.

@orowith2os orowith2os changed the title Split puppet.rs into a module and update enums. Split puppet.rs into a module Jun 30, 2023
@Speykious
Copy link
Member

So far the split is good. I think there's no need for a try_from file, there are like 2 small impl blocks for it.

Right now we implement TryFrom ourselves, which can be annoying when it comes to updating enum fields.
Strum handles this for us automatically, lessening the room for human error.

We can't use it right now if I understand this issue correctly, since we can't return a custom error type:
Peternator7/strum#13

Signed-off-by: Dallas Strouse <dastrouses@gmail.com>
@orowith2os orowith2os marked this pull request as ready for review June 30, 2023 10:19
@orowith2os
Copy link
Contributor Author

You underestimate just how annoyed I can get at code sometimes 😀

@orowith2os orowith2os marked this pull request as draft July 15, 2023 20:33
@Speykious Speykious added the pr:refactoring Code is being refactored label Jul 15, 2023
@Speykious
Copy link
Member

Speykious commented Feb 15, 2024

Since this has been hanging around, I've decided to not merge this. Kinda unnecessary at this point. Even for splitting Display I wouldn't do it, it's too much of a standard trait to implement for it to be separated from its struct, and the same can be said of TryFrom. (I know, I changed my opinion since then.)

I'll just let all the puppet structs be in one nice readable file like they always were.

@Speykious Speykious closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:refactoring Code is being refactored
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants