Skip to content

refactor: replace get_ methods with derive_getter::Getters #44

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

Conversation

lorenzoberts
Copy link
Contributor

When using the derive_getter lib it is not necessary to create one function for each attribute you want to expose with a get_ method. This commit adds the lib to Cargo.toml and replaces the existing get_ functions to a #[derive(Getter)]

Closes #33

@lorenzoberts lorenzoberts changed the title refactor: replace get_ methods to derive_getter::Getters refactor: replace get_ methods with derive_getter::Getters Sep 14, 2024
When using the derive_getter lib it is not necessary to
create one function for each attribute you want to
expose with a get_ method. This commit adds the lib
to Cargo.toml and replaces the existing get_ functions
to #[derive(Getter)]

Closes kworkflow#33

Signed-off-by: Lorenzo Bertin Salvador <lorenzobs@usp.br>
@davidbtadokoro
Copy link
Collaborator

Hi @lorenzoberts thanks a lot for this PR, going to take a look at it ASAP!

@davidbtadokoro
Copy link
Collaborator

davidbtadokoro commented Sep 18, 2024

@lorenzoberts. thank you so much for the effort you put into this PR! I imagine the trouble you went to assess where the getters are and replace them as I've noticed by reviewing this PR that I implemented methods/functions that have the get_ prefix but aren't getters 😬 (although the IDE certainly is a savior here).

I've tested it and reviewed it. It looks great and is almost ready for merging, IMHO 👍

Two points:

  1. I will take a more thorough look at this PR. As it has a lot of changed lines, you and I may have slipped some problems despite the nature of the change itself being stable enough that it probably won't introduce any regressions.
  2. You didn't cover the getters related to the Config type, which is not your fault, as I introduced them in a commit pushed later than your PR.

Both points above are on my end, so I feel like there is nothing to request on your end. I plan to amend the commit to cover the getters of Config and merge this change. In the case, I find anything serious, or that is worth bringing you aboard, I will ping you (of course, in case it goes smoothly, I will also ping you to see if you are OK with the amending).

@lorenzoberts
Copy link
Contributor Author

Thanks, @davidbtadokoro. It would have been much more difficult if you had created getter functions that didn't start with the get_ prefix, haha. So, props to you as well!
If you run into any issues with the two points you mentioned, feel free to ping me (online or offline), and I'll try my best to help.

Copy link
Collaborator

@davidbtadokoro davidbtadokoro left a comment

Choose a reason for hiding this comment

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

Hey @lorenzoberts, and thank really much for this PR! It is a great refactoring that cut up around 100 lines of code and made the project more idiomatic, for sure 👍

As I've mentioned in the previous comment, I had tasks on my end, which were fully reviewing the change as well as implementing these getters to Config. I also made some changes to remove the dereferencing of the usize (see the inline comment I left).

Although I said I would ping you to see the changes I've done before merging, I think they were simple enough, and I would want to block this any longer 😅

Anyway, thanks a lot! Change merged into the unstable branch 👍

Comment on lines -77 to +79
for mut patch in patch_feed.get_patches() {
for mut patch in patch_feed.patches().clone() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To tell you the truth, I spent quite a while thinking about adding the getter for the PatchFeed type to the detriment of having to clone here. At first, I went straight to redoing the old get_patch() that returned an owned type Vec<Patch>, but with the name patches().

However, I realized that PatchFeed is instantiated and soon dropped when the variable patch_feed of process_n_representative_patches goes out of scope. Combining the fact that, per request, we fill 200 patches (Patch not being a huge struct, itself), then soon drop it, I reverted to your approach 🤣

Comment on lines -110 to +113
if (patch_in_reply_to.get_number_in_series() == 0)
&& (patch.get_version() == patch_in_reply_to.get_version())
if (*patch_in_reply_to.number_in_series() == 0)
&& (*patch.version() == *patch_in_reply_to.version())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Including the other attributes that are of the type usize, I've noticed that the getters return an immutable borrow to the value, which isn't necessary as simple scalars like usize implement the Copy trait and trivially are copied to the stack. Since dereferencing is also a more "obscure" Rust feature, we should stick with the previous implementation but remove the get_ prefixes (as @OJarrisonn pointed out, this is the overall style).

I think this is the only real adaptation I did to your change besides covering the Config type getters. I've done this for the usize attributes of Patch and Config.

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.

2 participants