Skip to content

Conversation

silverjam
Copy link
Contributor

Description

@swift-nav/devinfra

See below.

API compatibility

Does this change introduce a API compatibility risk?

No, this message allows existing messages to be renamed with the _DEP or _DEP_A syntax so that they can be deprecated.

JIRA Reference

n/a -- see #1131 (cc @fpezzinosn)

Message ID changes are allowed as long as they are deprecations.
This means that the "old" message ID must be discoverable in the
current package with a "_DEP" suffix attached to it.
@silverjam silverjam requested a review from a team as a code owner May 5, 2022 23:50
jungleraptor
jungleraptor previously approved these changes May 5, 2022
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

@silverjam I might've given the thumbs up too quickly here.

If a message is deprecated, and another message takes it's place (name), does the new message still have to be backwards compat with the old one?

If not we might need to do more here. The messages are compared based on names, so after the validate_id check, it will still compare the fields of the name against the old name.

Edit: I looked at the PR and it looks like that new message is definitely not compatible with the old one.

We'll probably have to modify this validator script so that it keys the definitions based on their ID's instead of their names.

@jungleraptor jungleraptor dismissed their stale review May 6, 2022 00:23

Gave approval too quickly

@silverjam
Copy link
Contributor Author

@silverjam I might've given the thumbs up too quickly here.

If a message is deprecated, and another message takes it's place (name), does the new message still have to be backwards compat with the old one?

If not we might need to do more here. The messages are compared based on names, so after the validate_id check, it will still compare the fields of the name against the old name.

Edit: I looked at the PR and it looks like that new message is definitely not compatible with the old one.

New message doesn't need to be backwards compatible, but the old message (with the new name) needs to stay the same, so the same issue applies.

We'll probably have to modify this validator script so that it keys the definitions based on their ID's instead of their names.

I've update the tool to implement this, also applied black, mypy and pylint.

@silverjam silverjam requested a review from jungleraptor May 6, 2022 16:47
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

Two things but otherwise looks good:

  • I was under the impression that appending fields to messages was legal?
  • I thought it might be worthwhile to make the test case for a legal message deprecation change be the change that @fpezzinosn made in his PR. I was going to throw up a PR with that change on a different branch.

@silverjam
Copy link
Contributor Author

Two things but otherwise looks good:

  • I was under the impression that appending fields to messages was legal?

Yeah, I think it might work, but I don't think we've tested it yet, so I was thinking we should make it "illegal" until we can verify that it works and doesn't cause issues. There are certain types of "appending" that shouldn't legal, like having appending something after a variable sized array (but I don't think the generator will allow this either).

  • I thought it might be worthwhile to make the test case for a legal message deprecation change be the change that @fpezzinosn made in his PR. I was going to throw up a PR with that change on a different branch.

Ok, sounds good, I'm going to quickly test that this works with @fpezzinosn's branch before merging.

@silverjam
Copy link
Contributor Author

@isaactorz Any concerns? If not can I get a quick ✅?

Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants