Release 3.0.0 #214
Release 3.0.0 #214
Conversation
| @@ -6,7 +6,7 @@ | |||
| ], | |||
| "url": "https://github.com/wmde/WikibaseDataModelSerialization", | |||
| "description": "Serializers and deserializers for the Wikibase DataModel", | |||
| "version": "2.3.0", | |||
| "version": "3.0.0", | |||
There was a problem hiding this comment.
I would rather delete this line at all.
Packagist uses VCS repositories, so the statement above is very much true for Packagist as well. Specifying the version yourself will most likely end up creating problems at some point due to human error.
(From: https://getcomposer.org/doc/04-schema.md#version)
There was a problem hiding this comment.
This is not composer.json but extension.json.
|
This is not composer.json but extension.json.
Ops! Sorry. Awkward...
|
thiemowmde
force-pushed
the
release300
branch
from
9170865
to
5f48194
Compare
|
Huh, I did not even know you could specify a version in |
| * Changed the `DeserializerFactory` constructor to expect a list of Entity deserializers | ||
| * Changed the `SerializerFactory` constructor to expect a list of Entity serializers | ||
| * `DeserializerFactory::newEntityDeserializer` does not return default Item and Property deserializers any more | ||
| * `SerializerFactory::newEntitySerializer` does not return default Item and Property serializers any more |
There was a problem hiding this comment.
Speaking as maintainer of https://github.com/JeroenDeDauw/JsonDumpReader, I'm not happy with those changes. They make using this library harder for me and do not provide any benefit to me either. Why should I make updates to my code to accommodate features for Wiktionary and Commons that are not relevant to my tool that just needs to deal with items and properties?
There was a problem hiding this comment.
Personally I'm fine with keeping the two default instances. It's just that we had a lot of very similar places before, and found that in almost all cases it's more confusing than helpful to have such defaults. What happens is that a caller of such a factory function must know what it supports by default, and skip possible duplicates. Otherwise you will end with a dispatching (de)serializer with duplicate support for items and properties, and you can't even know which of the duplicates will be used.
How to solve this?
There was a problem hiding this comment.
I don't think I understand what you are talking about. Is it still relevant given these release note points where incorrect?
There was a problem hiding this comment.
#212 and the release notes here should have been one pull request. I revoked both.
The problem I'm describing above is still relevant, but can not be solved in this component. I believe it must be solved by not using the newEntity… methods, but constructing custom dispatching (de)serializers instead. I believe we will introduce a third top-level factory (in addition to the client and repo ones) for this that is aware of custom entity types.
Input on this subject is very much appreciated.
|
Seems like something went wrong here... release notes should be updated faster, same for the branch alias. The master branch contains breaking changes, which I'll pull in with |
This sounds like you have code that depends on master? I'm afraid I can't help you with that. Updating the release notes with every commit is a pain because it's guaranteed to conflict every single time. I know you are utilizing the editing feature of GitHub to update the master branch after a commit got merged. We (as in "the Wikidata team") are trying to avoid this in favour of a strict review rule, which is what this pull request is about. |
|
I just realized that master does not contain any changes, because the breaking changes described here got revoked. So I wonder what braking changes you are talking about? Did something happened between 2.2.0 and 2.3.0 that broke your code? |
|
My stuff does not depend on master. I was merely trying to figure out if my code would be compatible with the new version or not, and got confused by what I found. |
Well what you do is you choice, though here you have my view: release notes should be updated ASAP after a relevant change is made. If this is done via git or the GH UI does not matter to me, I use both. I also don't really care if you push directly to master or make a PR, as long as the PRs get merged very fast, so stuff does not go out of date and does not cause merge conflicts all the time. To me there is no reason to insist on code review for this. No code is modified. Release notes used to be on wiki, where there was definitely no CR. Why would it be different now they are in a .md file in git? |
|
If I may put my two cents in: What @JeroenDeDauw says here, especially the bit below, seems quite reasonable to me:
Is there really is a policy in "the Wikidata team" that updating release notes should be only happening directly before the release? I might not have been around long enough, but that was indeed a case in few releases I've seen happening. I wasn't aware this was a way of working we've actually are supposed to follow, I simply thought it just happened to be done that way. |
|
There was never such a policy. If anything, for our PHP libraries we used to do what I suggest. You can look in their version history to confirm that. Perhaps people forgot to do this since I left the team, and it started looking like this is how it's supposed to be done? Some people never really got on board with the whole idea of having release notes in the first place so this would not be very surprising. |
|
I talked to @thiemowmde the other day and I believe we came to conclusion we should be trying to rather be doing this the way you suggested. Well, at least I will be trying to stick to it. |
|
I don't know what "policy" you guys are talking about. Let me repeat one argument from yesterdays discussion: What we are talking about here is called "release notes", not "current state of master".
This is such a bad communication style on so many levels, I wish you would not do this. |
I meant that (with "this" you were referring to merge conflict in release notes file, but this is not very relevant here):
For the other bit:
This is true. On the other hand, as I mentioned when we talked yesterday, as we defined a "branch alias" in composer.json users of this package can get "current state of master" installed when they ask for 2.3.* (or whatever the current "branch alias" says). This is not making you're saying invalid but it is in my opinion an argument in favour of updating release notes when changes happen not just prior to making the regular release. |
Bug: T157960