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

Module splitting #31

Merged
merged 13 commits into from
Feb 19, 2025
Merged

Conversation

RustyNova016
Copy link
Contributor

I started splitting the modules into multiples ones according to #25. For now only Core is made, but I am creating this pull request for tracking and initial review

Here's some details:

  • I changed the response_type macro to have absolute paths and thus being able to be used in children modules.
  • I changed the way the separation comments are made. Now it is {request type} {apipath}, like shown in the official documentation. While the original ones are closer to string formatting, the new ones are easier to find with a ctrl-f from the official documentation.
  • I also added a link to the official documentation for quick referencing
  • The types have been sorted by order of appearances in the user documentation.

@InputUsername
Copy link
Owner

InputUsername commented Jul 30, 2024

Hi @RustyNova016, for the next release (0.8.1) I'd like to merge this PR first, but there are merge conflicts now. Would you be able to solve those? If not, feel free to close this PR and create a new one.

I really like these changes though, great job 👍

@InputUsername InputUsername modified the milestones: v0.8.1, v0.8.2 Dec 27, 2024
@RustyNova016
Copy link
Contributor Author

I finally got arround to finishing it.
But I gave up on the placeholders for the api responses. If anyone want free carpal tunnel, feel free to do it.

@InputUsername
Copy link
Owner

Thanks @RustyNova016 !! I will have a look later today.

But I gave up on the placeholders for the api responses. If anyone want free carpal tunnel, feel free to do it.

Haha, no problem at all. I'll see what I can do.

Copy link
Owner

@InputUsername InputUsername left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how you guys feel @RustyNova016 @shymega, but my personal preference for naming modules is <name>.rs over <name>/mod.rs.

With the latter, if you have multiple modules open in your editor they will all show up as mod.rs.

Thoughts?

Copy link
Collaborator

@shymega shymega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my personal way of doing modules is based on logical assertions.

  • If a module is limited to one module only, and does not use mod in the file, then $module.rs is suitable.
  • If a module requires other files in the same directly, then the module should be called src/$moduleName/mod.rs, and use mod $child/pub mod $child in $moduleName/mod.rs, and work that way.

@RustyNova016
Copy link
Contributor Author

I usually use the same logic as you @shymega, but here i's just future proofing.

Some pages might need to be split more, so I already made the mods ready to expand.

If it's an issue, I can put it back as single files. (It's pretty easy with the vscode extension)

@shymega
Copy link
Collaborator

shymega commented Feb 16, 2025

I usually use the same logic as you @shymega, but here i's just future proofing.

Some pages might need to be split more, so I already made the mods ready to expand.

If it's an issue, I can put it back as single files. (It's pretty easy with the vscode extension)

Yeah, I don't disagree with future proofing. I mean, we're getting a steady stream of contributions (thank you!), and I think it makes sense to start preparing for future additions, making new PR diffs smaller.

@InputUsername What do you think?

@InputUsername InputUsername self-requested a review February 17, 2025 12:50
Copy link
Owner

@InputUsername InputUsername left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After giving it some thought, I think this approach is fine. Everything looks good!

@shymega shymega self-requested a review February 19, 2025 22:24
Copy link
Collaborator

@shymega shymega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@shymega shymega merged commit 87eb6ba into InputUsername:main Feb 19, 2025
3 checks passed
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 this pull request may close these issues.

3 participants