feat: user id map in MigrationExchangeResponse as map #1821
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
=======================================
Coverage 81.76% 81.76%
=======================================
Files 35 35
Lines 7742 7742
Branches 316 316
=======================================
Hits 6330 6330
Misses 1400 1400
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
| warning?: string; | ||
| }; | ||
|
|
||
| export interface UseridMap { |
There was a problem hiding this comment.
If anyone was using that interface. Though given it was not very useful and cast was needed anyway, removing usage would be enough to fix it.
There was a problem hiding this comment.
How exactly is this a breaking change? Because the values within user_id_map were typed to any before, and now would be a string?
There was a problem hiding this comment.
I don’t consider this a breaking change. The auto-generated types here have been changed based on quicktype outputs over time. We've been trying to maintain backward compatibility as much as possible, but when an existing type is incomplete and/or invalid, changes like this can happen.
There was a problem hiding this comment.
Agreed. Perhaps in the future, if we start paying closer attention to the response types, and start augmenting them with manual types, we could employ a more stringent backwards compatibility policy for the response types.
There was a problem hiding this comment.
It's technically a breaking change regarding to type definitions as:
- Interface
UseridMapis exported. It's also included in publisheddist/files. So it's part of public API surface - Interface
UseridMapgets removed. Which is a breaking change in case someone was importing it & using it. - Type definition is narrower. Before it was very open (any non null type). Now it's an indexed type which is a stricter type. So it's also non backwards compatible.
But given the type needed a cast anyways and cast was needed it's probably not enough for a new major version bump due to this kind of small breaking change. And more if auto-generated types are following a good-enough approach.
I'm happy with whatever you decide, reasons sound fair enough to me :)
That is a problem, as when we go to generate the responses the next time, we will lose these changes, right? We have been discussing internally with @seratch about possibly combining the response type generation together with manual curation / integration with manually-crafted types in order to improve the experience (probably best shown off in @seratch's personal Slack client project's type generation here: for certain responses containing Block Kit elements, the generated types "link" up the correct, manually-curated If anything, I would like for us to use a similar approach to solve this issue: let the What do y'all think? |
|
Specifically talking about this PR (rather than general discussion about the future), if @davidlj95 just didn't include other unrelated changes, that's fine. When we run the generator script, other changes won't conflict with this PR. I understand @davidlj95 might hesitate to include other changes. If @davidlj95 made some manual changes to the generated outputs, those changes will be overwritten when any of us executes the script next time (as @filmaj mentioned). Either way, the generator script should generate code in an idempotent manner (as long as the JSON source files remain the same). Therefore, I'd suggest either:
As for the option 2, I can run the generator tomorrow too (if others don't have time in North American timezone today). |
|
I regenerated the latest response types in the branch |
hehe yup, don't want so much responsibility over me 😛
I didn't! I just discarded changes out of scope. But the change in this PR has been autogenerated by current script as @filmaj found out below (sorry was out for a bit and didn't get in time!)
I'd lean into option 2, we can close this if you want :) Just wanted to close the issue but there's no rush so happy for the issue to be fixed in next type update run Thanks folks :) |
Summary
Fixes #1584
In slackapi/java-slack-sdk#1325 , JSON samples were updated so that
quicktypeproperly infersuser_id_mapproperty type ofMigrationExchangeResponseas a type.Here the type is updated after running the response types generation script
Note that I manually discarded many changes after type generation: fields were added, new responses, ... In order to keep it in scope. Also some changes were related to code formatting and couldn't find the formatter tool & config used for the project if any.
Requirements (place an
xin each[ ])