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

Quote VERSION in VERSION_CONFIG #1207

Closed
wants to merge 3 commits into from
Closed

Quote VERSION in VERSION_CONFIG #1207

wants to merge 3 commits into from

Conversation

jonpas
Copy link
Member

@jonpas jonpas commented Aug 15, 2019

When merged this pull request will:

Cherry-picked from #1145.

@jonpas jonpas added this to the 3.13 milestone Aug 15, 2019
@jonpas jonpas requested a review from commy2 August 15, 2019 11:15
@@ -60,7 +60,7 @@
#endif

#ifndef VERSION_CONFIG
#define VERSION_CONFIG version = VERSION; versionStr = QUOTE(VERSION); versionAr[] = {VERSION_AR}
#define VERSION_CONFIG version = QUOTE(VERSION); versionStr = QUOTE(VERSION); versionAr[] = {VERSION_AR}
Copy link
Contributor

Choose a reason for hiding this comment

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

If versionStr is QUOTE(VERSION), then why should version be QUOTE(VERSION) as well?

Isn't it more likely that version is supposed to be a number, which works as long as there is just one comma inside the version number? After this we convert stuff like 1.00 to "1.00", even though that versionStr was already "1.00".

I don't think the choice of tool should dictate this.

I know it is a shitty way of writing this, but this we inherited from CBA_A2.

Copy link
Member Author

@jonpas jonpas Aug 19, 2019

Choose a reason for hiding this comment

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

Even CBA doesn't have a version number like that, but one with 2 dot separators, which means Arma quotes it anyways (thus is wrong).

Tool is not dictating this, Arma and the macro use in CBA and most mods is. Any proper tool will give you a warning for missing quoting. I thought stuff from A2 CBA is supposed to be rewritten anyways. ;)

I don't have a solution to provide backwards compatibility, but CBA should be without inconsistencies/warnings like this.

Copy link
Contributor

@commy2 commy2 left a comment

Choose a reason for hiding this comment

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

unclear

@commy2
Copy link
Contributor

commy2 commented Aug 31, 2019

Don't like this for its potential to turn NUMBER tokens into STRINGs for addons that use CBA and want to entry to remain the type it is.

Close?

@jonpas
Copy link
Member Author

jonpas commented Aug 31, 2019

As stated above, all mods using 3-number versioning already convert this to string, Arma does it.

This is technically wrong, Arma converts it, so it's not a number, and CBA shouldn't have magic conversions.

We could change it so this part only takes MAJOR/MINOR in mods (such as CBA itself), that way it will actually stay a number in all cases.

@commy2 commy2 closed this Aug 31, 2019
@commy2 commy2 deleted the quote-versioncfg branch August 31, 2019 12:04
@commy2 commy2 modified the milestones: 3.13, 3.12.2 Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unquoted string warning
2 participants