Closed BentEngbers closed 1 year ago
Went ahead and added review - mostly with implementation.
The flag is a bit odd, and not immediately clear what it does. It gets a bit verbose if fully describe, so probably more preference
auto_version_with_build_number
use_build_in_msix_version
Note: I'm not responsible for what's included. You'll need to convince @YehudaKremer if want to include this.
Thanks for your review!
Ive changed the name of the flag to auto_version_with_build_number
. I think that name is indeed more suitable for this feature.
Hello
I'm in holy days... I will check this hopefully in monday
Thank you for your contribution to this package 👍
The changes look good.
One thing I wasn't thinking about earlier was the String > int > String conversion was edge case of low version numbers. 1.0.0-5
returns 1.000.5.0
, whereas parsed int
would be 1.0.5.0
.
Looking through the code, the RegEx verification and Version.parse(...)
look like they'll behave fine. The path for the installer will contain the extra zeros (source):
'$_versionsFolderPath/${_config.appName}_${_config.msixVersion}.msix';
I'm not sure that'll cause any issues, but it is slightly different behavior.
Hello @BentEngbers
Thank you for investing more on the topic.
Unfortunately I don't see any sense in combining a build-number into another version part number, usually each part of the version symbolizes a specific thing.
I could be wrong and I'm sorry for that, but I want to leave the situation as it is, even though it is not ideal that there is an incompatibility between the stores it was inevitable after Microsoft Store decided not to use the build-number part.
Thanks for your efforts and your wonderful code, tests and documentation, but I think the current state is the best we can do.
resolves #185.
Although the issue has already been closed, no easy resolution has been implemented. I have modified my changes from my previous PR (#192), so that it implements this feature as described by @skyeskie in https://github.com/YehudaKremer/msix/issues/185#issuecomment-1544185408.
I have also updated the version documentation, so that this new flag is easy to understand and to use.
If more changes are required to merge this PR, i am happy to do that (Allow edits by maintainers is also enabled).