refactor!: move files and add infrastructure for system-level utils

[ivinjabraham]

This is a particularly large commit aiming to clean up the current codebase without code changes. Primarily achieves the following:

• Moves src/test_samples out of src as it is not code.
• Introduce infrastructure layer, which is for utilities that manage external dependencies such as logging.
• Use mod.rs inside module directories to clear up the crate root from having app.rs, handler.rs and so on.
• Use a more clear order of imports in all the files where module declarations (mod) are at the top, followed by external crate imports, std imports, then internal crate imports and parent module imports.
• lore was previously made public API; I'm not sure why. This has been removed.

The utility function binary exists was also inlined instead.

After the following changes, utils.rs just had loading_screen! left, and since we also had a global log_on_error! macro, I've renamed utils.rs to macros.rs

[davidbtadokoro]

That is a hefty PR, but with good intentions, IMHO. I will take the time to review it thoroughly, but while I do this, I have a couple of questions:

1. In the past, I thought that the use of mod.rs would indeed enhance code organization, but I found out that its use was unrecommended and that the argument for it was sound. Do you have any notion about it? If I am mistaken, I vouch for its use, but if the Rust community at large doesn't recommend it, maybe we should settle with these "horizontal" files (which I am as annoyed as you about them 🤣);
2. My Rust knowledge isn't the deepest and Rust itself is loaded with many details, but my intention with making lore public, was to make it possible to use it as a standalone lib crate. lore encapsulates most of the domain logic about the lore.kernel.org archives and I think other projects could leverage its implementations. However, if I am mistaken and we can make it private without losing this possibility, I don't a reason why not.

[davidbtadokoro]

[Quick Update]

I've solved the merge conflicts and tested from a user perspective, and everything works as intended. Also, the test suite still works seamlessly. With these big changes where much code is moved between places, I like to start with this to ensure that the refactoring didn't change the system's "external behavior" as it should (of course, if unit test coverage was good, this process would be much more reliable and quick 🥲).

I will start looking at the refactoring itself, even though I have a good high-level grasp of it due to solving conflicts and navigating the reformed codebase.

My take

This first step towards refactoring the architecture moved the project into cleaner organization, IMHO, and the infrastructure module makes a lot of sense!

I am just worried about the the two points I've raised in my previous comment about using mod.rs and about making lore lib crate not public.

Anyway, will make a thorough review ASAP.

P.S.: Great work!

[davidbtadokoro]

Don't know why I thought it would be hard to review this change, but it ended up being really fast. I just have a single comment about the commit message in "binary exists" where it should be together ("binary_exists").

Besides that, I thought it was strange to remove App::new(), but it does makes sense as it was only used for testing purposes.

In this sense, I reinforce that the only matters that are hanging are the ones I've commented before about the mod.rs and lore being public or not. Obs.: On the mod.rs note, if we choose to go this way, shouldn't we do this for everything (for example, with lore::lore_session).

[ivinjabraham]

That is a hefty PR

It's my fault for being so hard to review. I got distracted when fixing one thing after the other that I forgot to split things into different commits which would have made it much easier to review. Sorry about that.

1. In the past, I thought that the use of mod.rs would indeed enhance code organization, but I found out that its use was unrecommended and that the argument for it was sound. Do you have any notion about it?

Yep, the official Rust book doesn't recommend using mod.rs files anymore since it resulted in many mod.rs files which made it hard to navigate between files for some users and editors. However, this is still a pretty contested topic in the community. When I was contributing to Rust libraries, I found that they (Serenity and Tokio) don't follow the new approach and prefer the using mod.rs instead. IMO mod.rs is a lot more cleaner but I never really got confused between the files since I don't use a tabbed editor. I think the new approach is a lot more cluttered and looks a lot worse. I don't see any drawbacks of sticking with the old way of doing things, at least right now.

2. My Rust knowledge isn't the deepest and Rust itself is loaded with many details, but my intention with making lore public, was to make it possible to use it as a standalone lib crate. lore encapsulates most of the domain logic about the lore.kernel.org archives and I think other projects could leverage its implementations. However, if I am mistaken and we can make it private without losing this possibility, I don't a reason why not.

Ah, that is what I thought it was for too but then I wondered why it would not have been written as a separate library crate. I'm not 100% on this but if we use lib.rs to export the lore interface, then people would have to get the entire source of patch-hub (assuming this crate is published) to use the lore interface. It might also pull in our dependencies like ratatui. I think it'd be nice to export this as library, but maybe not within our own binary crate. Perhaps we can do something similar to tracing and use a Cargo workspace to have two crates—patch-hub and the lore interface? Or we can have another project and repo for the lore interface and make it more independent.

I just have a single comment about the commit message in "binary exists" where it should be together ("binary_exists").

Whoops, nice catch! Will ammend the message.

Besides that, I thought it was strange to remove App::new(), but it does makes sense as it was only used for testing purposes.

Yes, I was surprised clippy hadn't complained about this earlier. It was only after the refactoring that it started warning me.

Obs.: On the mod.rs note, if we choose to go this way, shouldn't we do this for everything (for example, with lore::lore_session).

Yes, I hadn't touched any of the lore code so I wasn't aware the same structuring dilemma existed there as well.

[davidbtadokoro]

Hey, @ivinjabraham, and thank you for the response and update. Below is my reply to your points. For the sake of brevity (which by now you know I don't have any 🙃), the points that I didn't answered are because I totally agree or have no comments about.

Replies

It's my fault for being so hard to review. I got distracted when fixing one thing after the other that I forgot to split things into different commits which would have made it much easier to review. Sorry about that.

No worries! I was saying it as a "justification" for needing more time to review this. It is normal for changes to grow big and commits atomicity sometimes gets obscured. Your PR wasn't hard difficult to review at all 😉

Yep, the official Rust book doesn't recommend using mod.rs files anymore since it resulted in many mod.rs files which made it hard to navigate between files for some users and editors. However, this is still a pretty contested topic in the community. When I was contributing to Rust libraries, I found that they (Serenity and Tokio) don't follow the new approach and prefer the using mod.rs instead. IMO mod.rs is a lot more cleaner but I never really got confused between the files since I don't use a tabbed editor. I think the new approach is a lot more cluttered and looks a lot worse. I don't see any drawbacks of sticking with the old way of doing things, at least right now.

Personally, I share the same vision as you in this matter. I understand the argument that having lots of mod.rs may confuse things, but we already adopted something similar with the tests.rs files. Having this encapsulation in directories is what makes the most sense to me and unarguably makes the hierarchy much cleaner. My problem was that, because I can't say I am a reference on Rust and the community view, I feared that my opinion could be inaccurate and I had the feeling that this wasn't such a debate and that the community agreed that mod.rs was a bad idea. With your experience in other Rust projects, I now feel confident in this approach.

In any case, I would like to hear the opinions of @OJarrisonn and @lorenzoberts on this matter. I know we already discussed this in the past, but given this new context, what do you guys think?

Next steps on my end

I wish to wait for our fellow contributors response before making any definitive move, but I am really convinced of this. In the meantime, I will resolve the new potential merge conflicts introduced due to the new PR merges and assert everything is working nice.

UPDATE: I will also take the liberty of mod.rsing the sub-modules of lore.

[nanaharris]

I'll always stand my ground against mod.rs. A file's name should show the file's intention and mod.rs on itself means nearly nothing IMHO. Also, it's a "deprecated" pattern so new projects should try to adhere to the new standards.

Btw, why just 1 commit? From your bullet list, it seems like each point could be a commit on its own

[davidbtadokoro]

Thank you so much for the attention, @OJarrisonn!

I'll always stand my ground against mod.rs. A file's name should show the file's intention and mod.rs on itself means nearly nothing IMHO.

I agree with it being ambiguous. We face the same issue with the files of same name inside app/screen/, ui/, and handler/, and also with the multiple tests.rs files, in the sense that we have to consider the full path to "resolve" the file in our heads. I disagree with the view that using mod.rs is undeniably a bad idea, though, as I see it as a trade-off, like with any design decision:

• With mod.rs, the pro is that it makes the file hierarchy cleaner, especially in projects with much nesting;
• With mod.rs, the con is that we need to consider the full path, as just mod.rs does not mean anything on its own, i.e., it isn't idiomatic, as you've said.

However (next reply)...

Also, it's a "deprecated" pattern so new projects should try to adhere to the new standards.

If this indeed is deprecated in the sense that rustlang will drop the support to it, then there is no discussion and we should avoid using mod.rs.

Nevertheless, I confess that I am getting a really mixed view on this matter as both have brought valid sources vouching for either side, and I also found mixed opinions in my research. I could've sworn that Rust The Book mentioned the use of mod.rs as deprecated or totally unrecommended, but it mentions it just as an "old-style" with the downside I mentioned above (as you can see here).

Btw, why just 1 commit? From your bullet list, it seems like each point could be a commit on its own

This is true, nice catch! My bad, I got caught up in the discussion of the decisions and didn't add these "high-level" review comments. @ivinjabraham, indeed we can split this commit into dedicated ones for each bullet point with the removal of binary_exists together with the infrastructure module commit.

If you need help splitting a commit, you can ping me and I will give you on how to do this (in my way, as there is many ways of doing things in git). Surely there are tutorials online, but I don't mind explaining how I do it.

[ivinjabraham]

I agree we should use the updated convention if mod.rs is deprecated. Though I can't seem to find any sources for this either.

As for splitting it up, is there an easier method than unstaging the work and using git add -p?

[davidbtadokoro]

Hi, @ivinjabraham, and thank you for the update!

As I mentioned in the previous comments, I agree with the idea and implementation for each of the 4 parts of the PR. Splitting the change is important (even more so with breaking refactors) for us to keep a good git history.

However, the middle commits about the infrastructure layer and removal of lore break compilation. Splitting a big change into smaller commits is sometimes complicated, so I understand that sometimes we get confused with which part should go to commit A or B 😄

Because commit 3 essentially modifies imports of lore, I think the root is that some things that should be on commit 2 are on commit 4 and vice-versa, i.e., you've made a mistake in splitting these two. One example is removing the binary_exists() function in commit 2 and substituting its calls to which::which() in commit 4.

There is one more complicator to this PR: the merged changes between when this was opened and now. Some recently merged PRs (one even by you) introduced some breaking changes for this PR...

Commit 1

Commit 1 about moving the test_samples dir looks legit to me, so I merged it into the unstable branch 👍

My suggestion about the rest of the PR

Make a git rebase --interactive and edit each commit. Make sure the compilation is working (both for bin and tests). Focus on stabilizing the current commit first, then fixing the conflicts that appear with each git rebase --continue;
Pull the latest upstream unstable, do a git rebase unstable, and incorporate the new upstream changes by solving the conflicts.

Conclusion

Sorry for being strict here. This is a breaking change to the codebase, so it is important to be really careful. Again, this isn't a pushback on the merit of the PR, as I am already sold on the idea and implementation.

[ivinjabraham]

I think I've fixed it now! I had to squash some latest changes on unstable that were missing since utils.rs got deleted into the first commit. I believe I've also sorted out the messed up changes in the commits and commits no longer break compilation.

[davidbtadokoro]

Hey @ivinjabraham, finally deep tested and gave a long thought (again) about this PR. IMHO, I still think this cleans a LOT the file hierarchy and the downside (many mod.rs files open at the same time) is manageable.

Thank you so much for the effort given here, the patience, and the protectiveness in bringing this idea, given that it is normally frowned upon at face value (I was on this boat).

Change merged into the unstable branch 👍