Feature/reduce duplication

[pelletier197]

Changes

I couldn't help but notice that there is a lot of duplication and redundancy across the SDK. Generally speaking, having copy-pasted code increase maintaining efforts and increase the probability of error for new maintainers.

So I bring a little suggestion to help reduce repetition by having the base entity class providing functions that allow implementations to customise a RequestBuilder and automatically build that builder.

Feel free to suggest improvements or refuse it if you don't like the idea.

Testing

I added this only to the Guardian Entity. The code here is completely covered by existing tests. No tests were changed.

• This change adds test coverage
• This change has been tested on the latest version of the platform/language or why not

Checklist

• I have read the Auth0 general contribution guidelines
• I have read the Auth0 Code of Conduct
• All existing and new tests complete without errors

[jimmyjames]

👋 hi @pelletier197, thanks for the contribution! There is duplication across the SDK and this looks like a good change on first glance. I'll look at the specific changes in detail next week, but just on first glance, do we need to make the new request builder public? Guessing we might need to since it's in a different package, or is there a use case for making the builder available to any user of the lib?

[pelletier197]

Guessing we might need to since it's in a different package, or is there a use case for making the builder available to any user of the lib?

I don't think there would be any need for the users to use this outside the library. People most likely only use the different Entity implementations and don't need to create requests themselves.

I did put the Builder where I believe it would fit better (close to the Request objects). If you prefer, it can be closer to where it's used instead if that makes more sense and make it package private.

[jimmyjames]

I don't think there would be any need for the users to use this outside the library. People most likely only use the different Entity implementations and don't need to create requests themselves.

I agree; it shouldn't be for public consumption until/unless there's a good reason.

I did put the Builder where I believe it would fit better (close to the Request objects). If you prefer, it can be closer to where it's used instead if that makes more sense and make it package private.

That makes sense, but to avoid tying ourselves to this implementation it's better to put it closer where it can be package-private. People shouldn't use it if it's documented as not for public-use, but ultimately as a public class it becomes part of the library's public API (and our backwards-compatibility check would fail if we did need to making a breaking change it at some point).

I'm headed out for holiday for the next week, I'll check back when I return. Thanks again!

[pelletier197]

Better late than never. I updated the PR to make the RequestBuilder package private 😃

[pelletier197]

Hey @jimmyjames! Did you get some time to review this? I'm on my way to implement a new entity for the Branding API, and I would really like to use this if possible.

[jimmyjames]

He @pelletier197, nso sorry for not getting to this sooner 😞. I like this change! I thought for a moment about reverting on my earlier comment about making the builder package-private, and instead putting it in an internal package so it can be shared with AuthenticationAPI... but that can be done later and let's not make perfect be the enemy of good. Looks like the codecov check is failing, so we probably have some code that needs tests here - let me know if you need help identifying where the coverage gaps are.

Thanks again for the contribution, your patience, and persistence 🙇

[pelletier197]

@jimmyjames Thanks for you answer. Correct me if i'm wrong, but I think that 100% of my changes are tested. The code coverage reduced because I reduced the amount of code in the project. Since there is less code, the percentage of uncovered code is simply greater... Am I missing something??

[pelletier197]

Any help here? 😅

[jimmyjames]

@jimmyjames Thanks for you answer. Correct me if i'm wrong, but I think that 100% of my changes are tested. The code coverage reduced because I reduced the amount of code in the project. Since there is less code, the percentage of uncovered code is simply greater... Am I missing something??

Yes, that makes sense. I'll try updating the branch and then approve and merge. Thanks!

[jimmyjames]

Thanks for this change! As discussed on the PR, the project coverage check is failing because of the code removal, so this is ready to be merged. Thanks!