Skip to content

Conversation

@joris
Copy link
Contributor

@joris joris commented Oct 4, 2018

No description provided.

@epels
Copy link
Contributor

epels commented Oct 5, 2018

Hi @joris, thanks a lot for opening a pull request! Good catch, and I definitely agree List should be namespaced as well. We do however need to maintain backwards compatibility, and I believe this is a breaking change - is this correct?

@joris
Copy link
Contributor Author

joris commented Oct 12, 2018

I think you’re right. I mean within the gem itself it doesn’t change anything because of how Ruby module nesting works. But indeed anyone using ::List directly, it will indeed break for them.
So you might want to merge this into some future release. Though generally people should only upgrade gems explicitly anyways and re-test their app after a a gem upgrade. But we have of course zero control over that.
To me it’s mostly important that it does get fixed. Because this is a bug in my mind (not an enhancement) and should therefore be fixed in the main gem at some point.

@epels
Copy link
Contributor

epels commented Oct 18, 2018

@joris That is indeed the issue here. I do agree consumers should not update their dependencies without checking what has changed, but we can't assume this unfortunately.
Even if we consider this a bug, there are people running this (successfully) in production right now, and changing this will break their code.
I will set the milestone to v2 for this.

@epels epels added this to the v2 milestone Oct 18, 2018
@karloscodes
Copy link

Any news about this?

@lucaong
Copy link

lucaong commented Feb 26, 2021

Is there a chance to merge this PR? I agree it's a breaking change, but only for people directly using the List class in their code (frankly very unlikely, as it's an internal utility class). On the other hand, it breaks any application that defines a global List class or module. As List is a pretty common name, one can expect that many applications could define such a class.

@jalerson jalerson mentioned this pull request Apr 3, 2022
@marcelcorso marcelcorso merged commit 75e564b into messagebird:master Apr 11, 2022
@marcelcorso
Copy link
Contributor

hey @joris! Long time no see! 👋

This finally got merged and released here
https://github.com/messagebird/ruby-rest-api/releases/tag/4.0.0

(I forgot to add it to the changelog 😞 )

thank you 🙏

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.

5 participants