Prerequisites for versioning. #156
Conversation
This adds a number of pieces required to generate version-adaptive code, but doesn't yet hook those pieces up. This allows testing against a corpus to ensure nothing is broken. A future commit will add version-adaptivity.
LottieGen/CommandLineOptions.cs
Outdated
| { | ||
| if (!uint.TryParse(arg, out var version)) | ||
| { | ||
| ErrorDescription = "minimum UAP version must be an positive integer"; |
There was a problem hiding this comment.
minimum UAP version must be an positive integer [](start = 48, length = 47)
Nit: Is this the established format for these? (First word not capitalized and no ending period) If you had the capitalization and period then you could more easily have them on the same line in the future if needed? #Closed
There was a problem hiding this comment.
I tried to make it more consistent, and got it half right. Will do what you suggest (capitalized and ending period). Note that we only output the first error during parsing of the command line anyway.
In reply to: 335220830 [](ancestors = 335220830)
LottieGen/Program.cs
Outdated
| @@ -67,6 +67,28 @@ RunResult Run() | |||
| _reporter.ErrorStream.WriteLine(_options.ErrorDescription); | |||
| return RunResult.InvalidUsage; | |||
| } | |||
| else if (_options.MinimumUapVersion.HasValue && _options.MinimumUapVersion < 7) | |||
There was a problem hiding this comment.
7 [](start = 85, length = 1)
In your next pass at versioning I assume you will define this 7 somewhere global and give it a good description and such. #Closed
LottieGen/Program.cs
Outdated
| else if (_options.MinimumUapVersion.HasValue && _options.MinimumUapVersion < 7) | ||
| { | ||
| // Unacceptable version. | ||
| _reporter.WriteError("Minimum UAP version must be 7 or above."); |
There was a problem hiding this comment.
_reporter.WriteError("Minimum UAP version must be 7 or above."); [](start = 12, length = 64)
Nit: maybe these error messages could say the version that was passed in. This would help for the scenarios where one person wrote the LottieGen automation script and another person needs to figure out later why things are broken. #Closed
| or higher and >= MinimumUapVersion. Code will be generated | ||
| that may take advantage of features in this version in order | ||
| to produce a better result. If not specified, defaults to | ||
| the latest UAP version. |
There was a problem hiding this comment.
the latest UAP version. [](start = 23, length = 23)
Do you mean the newest UAP version the OS supports or the newest version on the newest released version of the OS? #WontFix
There was a problem hiding this comment.
Discussed in person. The default acts as if you had specified the latest published version of the UAP contract. That doesn't mean that LottieGen knows about that latest version... it currently only knows up to version 8. But when 9 gets released, specifying 9 here would produce code that uses features in version 8, which is also what happens if you rely on the default.
I don't think we can make this clearer while keeping it concise, and I'm guessing it is ok as is.
In reply to: 335223414 [](ancestors = 335223414)
I meant to fix this but must have forgotten. It was a byproduct of some experimentation.
| internal CompositionGeometricClip CreateGeometricClip() | ||
| { | ||
| return _compositor.CreateGeometricClip(); | ||
| } |
There was a problem hiding this comment.
Should this one be switched back as well? #Resolved
There was a problem hiding this comment.
simeoncran
changed the title
This adds a number of pieces required to generate version-adaptive code, but doesn't
yet hook those pieces up. This allows testing against a corpus to ensure nothing is
broken. A future commit will add version-adaptivity.