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

Improve documentation of confusing newEntity(De)Serializer methods #222

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

thiemowmde
Copy link
Contributor

@thiemowmde thiemowmde commented Mar 24, 2017

This should be done before #220.

Bug: T160436

@lucaswerkmeister
Copy link
Member

Sorry, @manicki and I released 2.5.0 without noticing this PR… I’ll rebase it and change the milestone to 2.6.0. (In case the rebase is bad: the current commit hash is d669bc4.)

@thiemowmde
Copy link
Contributor Author

Thanks! Looks good to me. Do you think we can merge this now?

@lucaswerkmeister
Copy link
Member

That would probably be convenient just for the RELEASE-NOTES.md and composer.json changes (otherwise other pull requests have to do the same thing). Any objections from @JeroenDeDauw (regarding this comment)?

@manicki
Copy link
Member

manicki commented Aug 31, 2017

I note this references a Phabricator ticket closed as "invalid" which makes me suspicious :)

@thiemowmde
Copy link
Contributor Author

The ticket got closed because it asked for "removal" of the methods, which was never necessary. The problem was that the methods have been misused. These usages are fixed for a while now.

Right now the methods are unused, dead code. We can deprecate or remove them (as I already tried to do in #220) with pretty much no consequence.

Note that there is a single call to the now deprecated newEntityDeserializer method in Wikibase\InternalSerialization\DeserializerFactory::newEntityDeserializer. But this is also unused, dead code because said DeserializerFactory is never constructed with the third constructor parameter set to null.

Because of this usage a deprecation phase is the most convenient thing to do. Next step will be to remove the nullable feature from DeserializerFactory constructor.

@JeroenDeDauw
Copy link
Contributor

I am not enthusiastic about this change but can see how this code can be confusing in some situations due to decisions elsewhere. To me that is just another indication these decisions are dubious.

I'd be a lot more happy with just adding a note to these methods that states they are for the core Wikibase domain, and not other things that are now being added. Now I wrote this I realized it's probably better to specify this in the README as well, since this component only provides services for the core Wikibase domain (and if I'm not mistaken you are not planning on changing that).

As to the code being dead: I suspect I have PHP libs/tools that are using it (and doing so in a way that is not confusing).

@thiemowmde thiemowmde changed the title Deprecate confusing newEntity(De)Serializer methods Improve documentation of confusing newEntity(De)Serializer methods Sep 1, 2017
@thiemowmde
Copy link
Contributor Author

I removed the @deprecated tags and improved the documentation instead.

@thiemowmde thiemowmde mentioned this pull request Sep 15, 2017
Copy link
Member

@lucaswerkmeister lucaswerkmeister left a comment

Choose a reason for hiding this comment

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

I think we can merge this version, any objections @JeroenDeDauw?

@JeroenDeDauw
Copy link
Contributor

I stated I think this is not a nice change, you do with that feedback what you want.

@lucaswerkmeister
Copy link
Member

Does that apply to the current version as well, which doesn’t deprecate the methods but only updates their documentation? I thought this was what you suggested in this comment.

@thiemowmde thiemowmde modified the milestones: 2.6.0, 2.7.0 Sep 19, 2017
@lucaswerkmeister lucaswerkmeister merged commit 6ebaf85 into master Sep 20, 2017
@lucaswerkmeister lucaswerkmeister deleted the deprecations25 branch September 20, 2017 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants