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

Use STRING macros #6576

Merged
merged 10 commits into from
Dec 7, 2019
Merged

Use STRING macros #6576

merged 10 commits into from
Dec 7, 2019

Conversation

Wakbub
Copy link
Contributor

@Wakbub Wakbub commented Sep 16, 2018

When merged this pull request will:

  • Use STRING macros
  • Fix minor documentation typos related to STRING macros

Related, there is a problem with the naming of the strings in the concertina_wire module. They don't include the module name, so the string macros cannot be used.

"STR_ACE_CONCERTINA_WIRE"
"STR_ACE_CONCERTINA_WIRECOIL"
"STR_ACE_UNROLLWIRE"
"STR_ACE_ROLLWIRE"

If changing these strings, any mods using the current strings will be incompatible. But maybe change this in a bigger update for coherency?

@commy2
Copy link
Contributor

commy2 commented Sep 16, 2018

You can also write LLSTRING(var) for localize LSTRING(var)

@Wakbub
Copy link
Contributor Author

Wakbub commented Sep 16, 2018

Great. In that case the Coding Guidelines can be updated to reflect that.

@kymckay
Copy link
Member

kymckay commented Sep 16, 2018

I don't think we've ever worried about backwards compatibility for our stringtables, other mods could always copy them out if they want.

@Wakbub
Copy link
Contributor Author

Wakbub commented Sep 16, 2018

In that case, maybe this will do. Unsure how you want the strings in stringtable.xml. The practice of writing them differ even in the same module. From medical\stringtable.xml:

"STR_ACE_Medical_playersandai"
"STR_ACE_Medical_litterSimulationDetail"
"STR_ACE_Medical_NoInjuriesBodypart"
"STR_ACE_Medical_Triage_Status_Deceased"
"STR_ACE_Medical_medicalSupplyCrate_advanced"

|`LSTRING(banana)` | `"STR_ACE_balls_banana"` |
|`ELSTRING(leg,banana)` | `"STR_ACE_leg_banana"` |
|`LLSTRING(banana)` | `localize "STR_ACE_balls_banana"` |
|`LELSTRING(leg,banana)` | `localize "STR_ACE_leg_banana"` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should list all four imo.

@Wakbub
Copy link
Contributor Author

Wakbub commented Sep 16, 2018

Couldn't find any place where the actual string name (e.g., "STR_ACE_balls_banana") has been of interest and not the corresponding text, but there we go.

Also, if it aims to be a more comprehensive list, then it would make sense to include macros such as GETPRVAR, GETPAVAR, SETPRVAR, and SETPAVAR (for profile and parsing namespaces, when those for mission and ui namespaces are listed).

@commy2
Copy link
Contributor

commy2 commented Sep 16, 2018

We don't universally use those as opposed to LLSTRING etc, and I personally think they offer no benefit in i.e. readability, and only obscure the code.

@commy2
Copy link
Contributor

commy2 commented Sep 16, 2018

By that I mean that I wouldn't advertise for that family of macros, since I wouldn't use them myself for various reasons. There is no reason why the wiki should be a complete list of all macros that have been conceived of in the past that didn't establish themselves, because they offered nothing valuable enough for the price of being a macro.

@Wakbub
Copy link
Contributor Author

Wakbub commented Sep 16, 2018

Certainly. I never said complete however, just asked if the goal is for it to be more comprehensive. It can be tricky to know when the coding guidelines state one way to write, the first comment being there is another way to write, and then it is suggested that both ways should be listed. If both ways should be listed, it should be ok to use either way? Maybe your comment was just trivia rather than a suggestion.

As no one in ACE3 has ever used those macros mentioned though and you strongly advise against them, maybe they should not be defined? It would make sense to only have macro definitions that the contributors use and support. ACE3 and CBA_A3 are as far as I know the top places to learn what beautiful SQF code looks like, and anything to make it easier to learn for contributors and others is beneficial.

@commy2
Copy link
Contributor

commy2 commented Sep 16, 2018

That's just because LLSTRING is relatively recent, newer than the coding guidelines. I was for keeping LSTRING as well as LLSTRING, because sometimes you don't want to immediately translate strings via localize, for example when doing MP stuff where the string may be translated differently on multiple machines.

@commy2
Copy link
Contributor

commy2 commented Sep 16, 2018

maybe they should not be defined?

They have to be kept for bwc, and are probably used somewhere in legacy code. Or maybe someone else finds them useful and disagrees with me on wheter they should be used or not.

@Wakbub
Copy link
Contributor Author

Wakbub commented Sep 16, 2018

Ok, in case both macros serving a purpose and there's concerns for backwards compatibility all is good and this can be merged.

@PabstMirror PabstMirror added this to the 3.13.0 milestone Sep 22, 2018
@PabstMirror PabstMirror added the kind/cleanup Release Notes: **CHANGED:** label Sep 22, 2018
@jonpas jonpas merged commit 79e3de6 into acemod:master Dec 7, 2019
@PabstMirror PabstMirror modified the milestones: 3.13.0, 3.13.0-temp3 Dec 30, 2019
@PabstMirror PabstMirror added this to the 3.13.0 milestone Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Release Notes: **CHANGED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants