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 common Single Quotes for Strings #4939

Merged

Conversation

MaikBusch78
Copy link
Contributor

@MaikBusch78 MaikBusch78 commented Mar 6, 2024

Fixes NO issue, just cosmetic change

Changes proposed in this pull request:

  • Use single quoting for String constants

Test rendering with links to the example places:

  • tested with carto; the generated project.xml files before and after are identical

@pnorman
Copy link
Collaborator

pnorman commented Mar 6, 2024

Rendering should not be influenced by this cosmetic change

Have you tested this?

@MaikBusch78
Copy link
Contributor Author

Rendering should not be influenced by this cosmetic change

Have you tested this?

No

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

This needs testing - yes, this kind of change should not affect rendering - but we know from experience that Carto can be fairly particular when interpreting code - so we should not make assumptions here.

Regarding the idea of establishing stricter conventions on MSS coding - i am not opposed, but i think this should be documented in our coding guidelines then.

@MaikBusch78
Copy link
Contributor Author

Ok, i have tested it now by calling carto project.mml > project.xml.{number} on the original and on the changed quotes for string values version. Changing only the quotes from double to single the result is exactly the same (md5sum). To be sure that my changes have been reflected in the result i also tested to change the values. Changing entrance values from main to mainx resulted in many many changes in the xml. Changing leaf_type from leafless to leaflessx resulted in only a single line change.
From my understanding how the style is used, i believe that this kind of test is good enough.

@MaikBusch78
Copy link
Contributor Author

I have also updated the coding guidelines.

@MaikBusch78
Copy link
Contributor Author

@imagico I have seen now, you are also a maintainer. So i may ask you, what is left to do to make this PR merged?

@imagico imagico merged commit 3672c45 into gravitystorm:master Apr 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants