Azure devops commit serialization #1319
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1319 +/- ##
==========================================
+ Coverage 43.48% 43.49% +0.01%
==========================================
Files 23 23
Lines 2245 2247 +2
==========================================
+ Hits 976 977 +1
- Misses 1269 1270 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The CI seems to be failing, can you look into it? |
|
I have no idea what your tests are supposed to do, all I did is add the serialization of the field to Commit and change the serde attribute on the field in Release, not sure what any of that would have to do with the three failing tests that also produce no output. Not familiar with the GitHub interface for tests. |
|
Seems like there is a breaking change:
This variable is now Why are we renaming this to snake_case again? |
|
You used the annotation on the Release struct. This renames all the fields to camelCase but only on serialization, not deserialization. All the other fields with multiple words (commit_id, commit_ranges, submodule_commits) have annotations so I added one of those to this field too since I had to get it to use the same renaming on serialization and deserialization somehow and that was the way that affected the smallest number of fields. |
|
Obviously another option would be to replace the rename_all with something like to affect both serialization and deserialization but I don't know if that would affect any backwards compatibility or future plans for how you want to evolve this struct, it seems you are in the middle of a transition to camelCase? |
There was a problem hiding this comment.
I will reserve my judgment on whether this PR should be merged. However, I do think there is some merit in @taladar 's point regarding the inconsistency in the naming of the context field.
That said, the changes in this PR are not backward compatible, which poses a problem for git-cliff's SemVer practices. If the change is truly reasonable, I would recommend discussing it in an issue or discussion, including the timing of a potential release.
Regarding @orhun 's points:
Continuous Integration / Check NodeJS tarball:
This was already addressed in PR #1322, so it can be ignored here.
Test fixtures:
.github/fixtures/test-azure-devops-integration-custom-range/cliff.toml#L31
.github/fixtures/test-azure-devops-integration/cliff.toml#L31
These two test fixtures should be updated to reflect the changes introduced in this PR.
|
Technically the original change to expect a new field in the context JSON was not semver compatible either. I would assume any compatibility here should primarily be between the JSON generate by --context and consumed by --from-context within any given version which is currently not compatible. I don't particularly care which bits are renamed to resolve this but the current solution of producing camelCase and consuming snake_case is not sensible. And of course the other fix should be included either way, the one where Commit does not serialize the field at all currently. |
There was a problem hiding this comment.
Technically the original change to expect a new field in the context JSON was not semver compatible either. I would assume any compatibility here should primarily be between the JSON generate by --context and consumed by --from-context within any given version which is currently not compatible. I don't particularly care which bits are renamed to resolve this but the current solution of producing camelCase and consuming snake_case is not sensible.
And of course the other fix should be included either way, the one where Commit does not serialize the field at all currently.
Thanks for the clarification. Without concrete logs shared earlier in an issue, it took me some time to fully grasp the situation, but I think I now understand what the problem is.
From what I can see, the core issue is that the custom serializer for Commit does not serialize the azure_devops field correctly. Therefore, as mentioned in my review, I believe the required fix is confined to that single place.
UPDATE (22/12/2025):
I've reviewed this PR again, taking into account the existing conventions, and I believe the changes in this PR are consistent with them.
That said, the variable previously used as azureDevops in templates should be azure_devops to match the existing conventions. Consequently, the relevant test fixtures will need to be updated accordingly. See:
| #[cfg(feature = "bitbucket")] | ||
| commit.serialize_field("bitbucket", &self.bitbucket)?; | ||
| #[cfg(feature = "azure_devops")] | ||
| commit.serialize_field("azure_devops", &self.azure_devops)?; |
There was a problem hiding this comment.
Using Camel Case in JSON is the established convention. Can we change this to azureDevops instead?
This looks good to me.
| pub bitbucket: RemoteReleaseMetadata, | ||
| /// Contributors. | ||
| #[cfg(feature = "azure_devops")] | ||
| #[serde(rename = "azure_devops")] |
There was a problem hiding this comment.
Do we really need this change?
This looks good to me.
|
It is actually two separate fixes. The one you mention and the other problem is that the Release struct has a rename_all annotation that only applies to the derived Serialize but not to the derived Deserialize implementation, keeping the snake_case name for Deserialize while using the camelCase name in Serialize. |
Ah, right. Thanks for the clarification, @taladar . @orhun , why was this implemented asymmetrically? I would have expected something like: #[serde(rename_all = "camelCase")]so that both See: |
4 participants
see #1318
Basically the Commit serialization was missing the new azure_devops field entirely in its manual Serialize implementation and the Release serialization applied its renaming of all fields to camel case only on serialization, the other fields that would have been affected by that had manual rename attributes on the field level. It might make sense to rethink if renaming all fields to camel case makes sense at all if all fields with multiple words manually revert that anyway.