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

Don't define structs as top-level constants #183

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

makimoto
Copy link
Contributor

@makimoto makimoto commented Jul 24, 2019

Changes

Structs defined in lib/auth0/mixins are currently defined as top-level
constants (e.g. AccessToken). However, these names are popular and can
conflict with existing classes and break the client application.
Therefore, I'd like to put them as children of Auth0 module.

References

Closes #201

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of Ruby

Checklist

@makimoto makimoto requested a review from a team July 24, 2019 08:38
@joshcanhelp
Copy link
Contributor

Thanks once again @makimoto!

The problem here, though, is that this would be a breaking change for anyone using these structs, correct? I think this is a positive change but would have to be released in a major (if I understand correctly).

@makimoto
Copy link
Contributor Author

@joshcanhelp Probably, yes.
In this patch, I changed AccessToken, ApiToken, and Permission.
If my understanding is correct, AccessToken and ApiToken are used as return values, so they may not break compatibility unless a client application checks their types. (e.g. if response.is_a?(AccessToken))
However, Permission is used as a parameter for some methods (e.g. add_role_permissions) so it requires modifications at client applications' end.

@joshcanhelp
Copy link
Contributor

@makimoto - The Permissions struct can change because that's not been released yet, I'm fine with that. For the others, it's probably best to just add a # TODO there and we'll address in a major.

@joshcanhelp joshcanhelp added this to the v5.0.0 milestone Aug 1, 2019
@joshcanhelp joshcanhelp removed the request for review from a team August 1, 2019 21:14
@stale stale bot added the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@auth0 auth0 deleted a comment from stale bot Jan 6, 2020
@stale stale bot removed the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@joshcanhelp joshcanhelp self-assigned this Jan 6, 2020
@pjo336
Copy link

pjo336 commented Feb 7, 2020

What's the status on this? Anything we could do to move it along? We have some conflicts

@zinosama
Copy link

zinosama commented Feb 7, 2020

Hey @joshcanhelp the Permission struct change is breaking our application. What are the remaining blockers before we can get this merged? Happy to help move this forward 😄 Please let me know.

@qortex
Copy link
Contributor

qortex commented Apr 6, 2020

Any chance that could move forward?

@copiousfreetime
Copy link

I also just encountered this issue and was about to do a pull request for it and saw this. Please do move this forward and release a 5.0 version.

@qortex
Copy link
Contributor

qortex commented Apr 8, 2020

I also just encountered this issue and was about to do a pull request for it and saw this. Please do move this forward and release a 5.0 version.

That would be great, but it seems Auth0 has dropped support for this gem, or at least designated maintainers are no longer taking care of the repo.

@joshcanhelp Can you share Auth0 plans? Should we fork and go on with it ourselves? Thanks for your visibility on this matter.

@davidpatrick
Copy link
Contributor

Hey everybody, we apologize for the delay on this. We have kept putting this off until we had more planned for a 5.0 release. Considering that we still don't have anything major planned for this SDK, I will move forward with this and #171 for a 5.0 release.

Structs defined in lib/auth0/mixins are currently defined as top-level
constants (e.g. `AccessToken`). However, these names are popular and can
conflict with existing classes and break the client application.
Therefore, I'd like to put them as children of `Auth0` module.
@davidpatrick davidpatrick merged commit ebc3068 into auth0:master Oct 20, 2020
@qortex
Copy link
Contributor

qortex commented Oct 21, 2020

Great, thanks for your work!

@davidpatrick davidpatrick mentioned this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permission Struct Conflicts with Model
7 participants