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

config: Allow equal signs (=) between variable names and value #926

Closed
wants to merge 1 commit into from

Conversation

Moguri
Copy link
Collaborator

@Moguri Moguri commented May 1, 2020

This allows for more INI-style syntax as proposed by #523. Sections are
still not supported.

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #926 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #926      +/-   ##
==========================================
+ Coverage   14.99%   15.01%   +0.01%     
==========================================
  Files        3701     3701              
  Lines      355662   355668       +6     
==========================================
+ Hits        53339    53390      +51     
+ Misses     302323   302278      -45     
Impacted Files Coverage Δ
dtool/src/prc/configPage.cxx 62.43% <100.00%> (+3.64%) ⬆️
panda/src/cocoadisplay/cocoaPandaApp.mm 0.00% <0.00%> (-29.17%) ⬇️
panda/src/cocoadisplay/cocoaGraphicsWindow.mm 6.74% <0.00%> (-0.37%) ⬇️
panda/src/putil/bamCache.cxx 34.45% <0.00%> (+0.17%) ⬆️
dtool/src/prc/configDeclaration.cxx 52.77% <0.00%> (+0.92%) ⬆️
dtool/src/prc/notifyCategory.I 47.82% <0.00%> (+4.34%) ⬆️
dtool/src/prc/configDeclaration.I 59.75% <0.00%> (+6.09%) ⬆️
dtool/src/prc/configVariableCore.cxx 58.20% <0.00%> (+14.81%) ⬆️
dtool/src/prc/configFlags.cxx 60.00% <0.00%> (+60.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f1289b...9cfc090. Read the comment docs.

@rdb
Copy link
Member

rdb commented May 1, 2020

Interesting. I think this PR would allow var = = value, though?

I think the is_split_character, which is not declared externally, should be marked static (since it's not declared elsewhere, no point in giving it external linkage), and hoisted to the top of the .cxx file by Panda convention.

Do we want to enable this optionally, as part of a special mode for now (possibly toggled by extension), or do we want to allow this syntax for all .prc files?

@Moguri
Copy link
Collaborator Author

Moguri commented May 1, 2020

Yes, it allows any combination of spaces and equal signs. I figured this would be the simplest approach and PRC isn't very strict as is (e.g., anything start with "t" being "true").

I'll fix up the scope of is_split_character() and move it.

@Moguri
Copy link
Collaborator Author

Moguri commented May 2, 2020

I forced pushed a new version that only allows one equal sign.

This allows for more INI-style syntax as proposed by panda3d#523. Sections are
still not supported.
@Moguri
Copy link
Collaborator Author

Moguri commented Aug 6, 2020

@rdb Is this PR something we want? What is missing to move this along? More discussion?

@rdb
Copy link
Member

rdb commented Aug 10, 2020

One question I raised for discussion remains unaddressed, and I'd like to carefully consider that before we make this otherwise irreversible change.

@Moguri
Copy link
Collaborator Author

Moguri commented Aug 10, 2020

I am going to abandon this PR for now and we can continue discussion in #523. I started this PR in the hopes of quickly getting an issue closed, but I am otherwise not strongly invested in the feature. The code will still be around for someone to pick up.

@Moguri Moguri closed this Aug 10, 2020
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

2 participants