Suggestion on the handling of deprecated fields

[cjbearman]

I'd like to make a suggestion regarding the data definition for deprecated fields, for example: https://github.com/prusa3d/OpenPrintTag/blob/9fa7e5e2ac2686feea1ca52c0016fa37bd631520/data/main_fields.yaml#L112

I have been working on an implementation of this standard in golang (https://github.com/cjbearman/openprinttag). I have chosen to use code generation to generate representations of the different tag regions, giving the user definitive methods to retrieve and set each field value.

My concern is that the representation of the deprecated fields appears to be just the declaration that the field used to exist but has been deprecated - information about the name, type and so forth has been omitted.

In my opinion, and certainly as it relates to my implementation, this choice has the potential to cause issues in the case where code generation is used. Specifically, should a field become deprecated and the prior record of type, name and other associated parameters is removed from the definition, code generation would no longer be able to include the representation of the field in it's generated code. This causes issues to people using the code when they update to the new version, because functions previously available related to the field will no longer be present, thus causing compilation failure and the need to update code.

In my opinion, it would be better to retain the full context for each deprecated field, and simply mark it as deprecated. In this way, comments and deprecation flags can be added by code generation to the associated functions, to alert the user. In addition, checkers (such as opt-check) can provide more meaningful insight if the field continues to be used.

[XAMPPRocky]

FWIW I've also been writing a library implementation using code gen and also ran into this issue.

It does have a downstream impact of causing a breaking change in libraries and it would pin implementations to older library versions if they need to support deprecated fields.

Obviously less of an issue right now because of how green field the project is, but I think down the road it will be a pain point.

[CZDanol]

This causes issues to people using the code when they update to the new version, because functions previously available related to the field will no longer be present, thus causing compilation failure and the need to update code.

Is this a bad thing? If you update the library, that implies that you want to be up-to-date with the newest standard.

Say that the deprecated field was an enum. For things to work, you would then also need to keep the file in the enum values in the repository. You could then end up with a lot of dead code in the specification.

In code libraries, functions are first marked as deprecated, and then they can also completely removed after some time. While I agree that it is a good idea to leave the field around for a bit when marked as deprecated, I would say that ultimately, we do want to fully remove it from the spec.

[XAMPPRocky]

Is this a bad thing? If you update the library, that implies that you want to be up-to-date with the newest standard.

Library versions are different from standard versions. There is a lot of functionality that you want to update and not have it affect what standard you're supporting.

Let's say I release 1.0 of my library, you use that, then in 2.0 I update the standard, removing fields your spools use, then in 2.1 I fix a security vulnerability in the CBOR logic.

If you want that CBOR fix you have to either break your spool compatibility, or backport the fix to your version, neither of which is a good user experience.

While I agree that it is a good idea to leave the field around for a bit when marked as deprecated, I would say that ultimately, we do want to fully remove it from the spec.

I don't think removing it from the spec is good idea, not only does it make it harder to support the spec from a library perspective, but this would also introduce an artificial shelf life to the smart spool features.

Let's say as an example the new filament drying temp property. Let's say I encode a spool with that property today, and a year from now the standard deprecates it because they need to update the unit of measurement, then my slicer which uses the new library version can't read that old property without adding special casing or dropping support for those spools.

This could mean that users/customers could be sold a spool with a set of features which are no longer true as the standard updates and downstream changes.

I think as support for OPT grows there's going to be a growing number of spools in the wild that feature deprecated properties, and developers should be able to still support those properties even if they are no longer best practice.

[CZDanol]

All right, we will consider this further.

However, IIRC the fields that are deprecated in the spec now have been deprecated even before the standard was released, so there is no benefit in leaving any data about them in the standard whatsoever. The deprecated info record there is only to prevent clashes with in a few debug spools that we were playing around with.

Maybe we should drop the record fully then from the spec?

[cjbearman]

Maybe we should drop the record fully then from the spec?

I have no hard opinion on whether to remove the existing deprecated fields from the specification. I would say, however, that the existing deprecations did prove useful from my perspective in identifying deprecation as something that needed to be handled when parsing the definitions.

[CZDanol]

Hmm, okay. For now then, I'd leave the deprecated empty fields there to remind people that there is such a thing, and we will try to be careful when deprecating in the future :)