docs: add the diff standard #9
Conversation
There was a problem hiding this comment.
Awesome stuff!
Very good take on taking the user perspective here imho.
For better review, could you please introduce another section where you group all breaking and non breaking changes. So reviewer instead of going through all props to see what is breaing, can actually just check out all breaking props in one group? I mean:
## Breaking changes
- Change in `version` in `info`
- Change in `url` in `server`
- `Server` object removal
Also please clearly state in the intro that atm bindings are out of scope for diff library as these are yet not mature enough and lack proper tooling support
I love the idea of a machine-readable diff-map which can be applied to a target document. Thus (if there was a JSON Schema for the diff-map object) we could create one for other JSON/YAML based standards (cough) not just AsyncAPI. My only query would be about treating a change in |
There was a problem hiding this comment.
Looking good @aayushmau5 👏
I would love if the document defines the thought behind when a change is considered non-breaking vs breaking.
For example, any change to id might not be considered a breaking change in general (again depending on the thought behind the diff), but it depends on the use case. If a user uses the id to generate specific code that suddenly breaks because for that user id IS a breaking change.
As @MikeRalphson touched upon, I suggest that, while we define a standard diff, it should be adaptable and modified by the user, which means instead of hardcoding/forcing what is breaking and what is not, maybe it can be done using a configuration? At least consider whether it would be worth investing in from the start, maybe it won't be that difficult compared to hardcoding it.
I completely agree with @MikeRalphson that changes to $ref cannot on its own be considered a breaking change. It needs to be realized first.
standard.md
Outdated
| 1. Change in `traits` in `subscribe`|`publish` | ||
| 1. Change in `contentType` in `message` | ||
| 1. Change in `name` in `message` | ||
| 1. Change in `payload` in `message` |
There was a problem hiding this comment.
To me this cannot be stated, what if you change the description of a property when using JSON Schema? 🤔 That should not be considered a breaking change.
What's the reason behind assuming changes to payload is breaking?
I could see that this is just for more easily assume it, as you then would need to diff JSON Schema as well, which is gonna take more time 😅
There was a problem hiding this comment.
I completely overlooked that point, but you are right, changing the description and such in message payload(inside a JSON schema) is not a breaking change. Will update the document :)
There was a problem hiding this comment.
I think we could try to define a basic minimum of non-breaking changes in JSON Schema in payload and threat the rest as breaking, or? later it could evolve into something separate? or we are complete excluding payload content from Diff because AsyncAPI allows also payloads with Avro and other formats? This way we would always treat change in payload as breaking, clearly documented and explained of course
There was a problem hiding this comment.
I think we could try to define a basic minimum of non-breaking changes in JSON Schema in payload and threat the rest as breaking, or?
I thought about this. Like, we can define a set of properties as non-breaking and mark others as breaking. But I'm not too sure if we should do that or not.
we are complete excluding payload content from Diff because AsyncAPI allows also payloads with Avro and other formats?
I don't think so. Like @jonaslagoni said, changing the description will throw us a "breaking" change error where it is not.
But then again, do we want to go through the whole JSON Schema and mark everything as breaking/non-breaking?
If that's the case, we can go with the first approach. Only put "non-breaking" properties in the standard, and treat everything else as "breaking"
There was a problem hiding this comment.
both solutions are bad and not something user can rely on. Wouldn't it make sense to have 3rd category like, uncategorized? or unknown?
There was a problem hiding this comment.
Yeah, I think having an unknown or uncategorized category will be good.
So, what message do you think should uncategorized convey? I'm thinking of something like "these changes are uncategorized, the user should handle these themselves".
There was a problem hiding this comment.
So, I have made some changes to the standard. Now it includes unclassfied changes as well. Right now, the message headers and payload are being marked as unclassified.
My 2c on whether $refs are breaking changes... ideally I'd like to see both, something indicating that the $ref is broken, and something indicating that all those things that reference are naturally broken. This prompt is based on how validation that occurs on the resolved spec can be noisy as hell. As well as the fact that the breaking change may being address/fixed in one place, not many -- both in code (if the $ref'd model is reusable) and in the definition. </2c> |
@jonaslagoni Working on it :)
Interesting. You are right, it might depend on the use case whether a change in breaking or not.
I really like this idea. I'm thinking of having some defaults configs that come with the package, but it can be easily overridable. So, In case if someone doesn't like the default configs, they can provide their own config, which our diff will use instead of using the default one. Any thoughts?
Yup, I think you are right. The parser will inline those |
There was a problem hiding this comment.
did you already think on how this standard would be coded?
did you consider that what you have here could be later declared in a yml file that user can override as a config?
example yaml file would have info.license on a list with a description that it is breaking because the license of the API changed that needs to be reviewed before the user uses the API further. It would be marked as breaking. But user would be able to provide alternative yaml where this is marked as non-breaking
Kind of. Will need to discuss on that as well, but right now, as a POC, I have a So, if a user provides a json or yml(which we can convert to json), we will merge that change into the JSON using some JSON merging technique. |
|
@aayushmau5 yup, a definitely separate topic, good for another GitHub Issue 😄 and something that can be added later, which will be easy if you already take config from a json |
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
@aayushmau5 Can you please also talk about how are we going to present these changes to the user?
|
Hey @vinitshahdeo There's an ongoing discussion about the output in this issue: #14 Definitely need your input there :) |
|
@aayushmau5 what plans do you have with this PR? |
|
@derberg I was thinking about this, and I'm not too sure what to do with this document. If we only care about the categories of changes, one can see them inside our standard object. I guess we can use this document to justify why a change has been considered breaking, non-breaking etc. What do you say? |
|
for sure useful part is how we classify changes and why but rest is redundant with the standard I think. Maybe just move some of it to readme and that is it, and in the readme link to standard after initial explanation |
|
Closing this one. Main points that should be addressed will be addressed in the README. |
In order to categorize a certain change to AsyncAPI document as "breaking" or "non-breaking", we need to have a "standard" which documents what changes can be considered "breaking" or "non-breaking".
When the community agrees on this "standard", it will be mapped to a
JSONfile which will contain all the information in here, which the diff library can utilize to categorize changes.Please note that the standard has been written from the Point of View of a Consumer(client/server), thus some changes were considered as "non-breaking" because the consumer won't be affected by that change.