Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation of DSDL definitions #10

Closed
kjetilkjeka opened this issue Aug 9, 2018 · 6 comments
Closed

Deprecation of DSDL definitions #10

kjetilkjeka opened this issue Aug 9, 2018 · 6 comments

Comments

@kjetilkjeka
Copy link
Contributor

Split out the deprecation discussion into this thread (why do we always seem to wander off topic)

The state of the discussion when split up

Pavel:

I have added a @deprecated keyword to the current draft a couple of weeks ago; it's supposed to be translated into language-specific deprecation markers, e.g. [[deprecated]] in C++ or DeprecationWarning in Python.

Kjetil:

How do we express deprecation?

Let's address this concern before moving on to the bigger concerns.

I have added a @deprecated keyword to the current draft a couple of weeks ago; it's supposed to be translated into language-specific deprecation markers, e.g. [[deprecated]] in C++ or DeprecationWarning in Python.

That's nice. I was playing around with a similar idea myself. What i was thinking about was adding
@lifecycle <pre-release|released|deprecated|removed>, this way we can add a new type without releasing it, yank a buggy type and deprecate old types. This is how i vision it would work.

  • Pre-realease - do not generate code for this definition unless some dangerous keyword is presented.
  • Released - As normal (as it currently works)
  • Deprecated - Issue a warning that the type is deprecated and will be removed in the future.
  • Removed - Issue an error that this type has been through the deprecation process and is no longer usable.

Even if we initially only supported deprecated It would allow us to add other lifecycle items (like removed) later on without making a new directive. Both our solutions would of course require the directive related to deprecated to be an exception to the immutability guarantees together with comments and whitespaces (which is of course totally fine).

Pavel:

I like the flexibility argument, but I have two questions:

  1. What's the point of the removed option if the definition can be just physically removed to the same effect?
  2. pre-release goes against the versioning guarantees that once something is published it can't be removed or changed without a proper deprecation process. If you want tentative volatile definitions, use version 0. I think we can make this work but that would be at the cost of extra complexity in the specification, so I am against it.

If the two options mentioned above are removed, we're back to just deprecated and released. I think the meaning of the keyword @lifecycle might be slightly less transparent than @deprecated, i.e. somebody who is not familiar with the UAVCAN specification might fail to understand what it means; @deprecated seems more approachable. Also, most published data type definitions will be in the release stage, meaning that it would make sense to assume release by default if not specified, so @lifecycle release becomes redundant.

I suggest keeping @deprecated.

@kjetilkjeka
Copy link
Contributor Author

What's the point of the removed option if the definition can be just physically removed to the same effect?

The main idea was to give information about potential DTID/name/path conflicts for future definitions while failing users of the definition to compile. We should probably at least keep this element out of the mix until we can see what the dsdl checker ends up doing in a bit more detail.

pre-release goes against the versioning guarantees that once something is published it can't be removed or changed without a proper deprecation process. If you want tentative volatile definitions, use version 0. I think we can make this work but that would be at the cost of extra complexity in the specification, so I am against it.

Well my intention was to not consider it "published" before it reached released. The pre-release was meant as a way for early testers to evaluate 2.0 definitions and the like before released.

It's actually a bit different than 0.x definitions for a few reasons:

  • It's quite more dangerous since it may conflict with released stable versions. This would need to make it very hard to enable by mistake.
  • It should be thought more of as a way to develop the type replacing the current.
  • It can be used with "stable" types
  • It can even be used with 0.x types to

My counter proposal would be to use @lifecycle <released|deprecated> with an implicit default to @lifecycle deprecated. This would allow most people not to think about it while allowing us to add more stages at a later point.

With that said I also totally accept @deprecated. We should in any case leave this issue open for a week or two in case someone else wants to bikeshed this further (Hey! perhaps @deprecated should be an alias for @lifecycle release 😆 ). I won't object to you closing this thread with the decision you're in favor for when you feel it has cooked long enough, unless you want to discuss it further.

@pavel-kirienko
Copy link
Member

The main idea was to give information about potential DTID/name/path conflicts for future definitions while failing users of the definition to compile. We should probably at least keep this element out of the mix until we can see what the dsdl checker ends up doing in a bit more detail.

I would like to propose a general principle we should follow: one should get a full understanding of the current DSDL set just by looking at the list of files in the DSDL namespace directory, without having to look inside the files. That would rule out things like @lifecycle removed and @lifecycle pre-release.

Specifically, I don't think that @lifecycle removed is necessary because by the time the data type is removed it has already been deprecated for at least two years, giving users supposedly sufficient time to upgrade their systems and move away from it. That means that by the time it is physically removed, its DTID can be safely repurposed immediately.

As for @lifecycle pre-release - I see the value, but again I'm not sure whether the added complexity would be worth it. As an alternative, we could explore lifting the compatibility guarantees from data type definitions where the minor version number is 0.

I think that we should strive to keep definitions as clean and simple as we can. That means that the most commonly used state shall also be the default state. Otherwise, we'll end up with boilerplate code, requiring virtually every definition to contain @lifecycle released. I am very much against that.

I suggest accepting @deprecated, released-by-default, and keeping in mind the possibility of adding new directives in the future.

@kjetilkjeka
Copy link
Contributor Author

kjetilkjeka commented Aug 14, 2018

I suggest accepting @deprecated, released-by-default, and keeping in mind the possibility of adding new directives in the future.

As said before, I see the two alternatives as more or less equal and I'm totally OK with this.

👍

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 14, 2018

Looks like we have a consensus here

I am curious what's your take on this:

As an alternative, we could explore lifting the compatibility guarantees from data type definitions where the minor version number is 0.

@kjetilkjeka
Copy link
Contributor Author

kjetilkjeka commented Aug 21, 2018

As an alternative, we could explore lifting the compatibility guarantees from data type definitions where the minor version number is 0.

I don't think this is a good idea. It goes against what users will expect when it comes to versioning.

@pavel-kirienko
Copy link
Member

pavel-kirienko commented Aug 26, 2018

Okay. Do we consider this one resolved?

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

No branches or pull requests

2 participants