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

Fix #428 #429

Merged
merged 3 commits into from Aug 13, 2020
Merged

Fix #428 #429

merged 3 commits into from Aug 13, 2020

Conversation

Hackerpilot
Copy link
Collaborator

This fixes a few logic problems in the handling of the trueStyle and falseStyle fields. The worst of these is that the old code called advance to skip over a token before then checking what the token was by looking at the new current token. It also had logic to set the field to DeclarationListStyle.single inside of an if that checked that there was a { or :, which makes no sense.

@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #429 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #429   +/-   ##
=======================================
  Coverage   81.85%   81.86%           
=======================================
  Files          11       11           
  Lines        8443     8446    +3     
=======================================
+ Hits         6911     6914    +3     
  Misses       1532     1532           
Impacted Files Coverage Δ
src/dparse/astprinter.d 94.77% <100.00%> (ø)
src/dparse/parser.d 91.20% <100.00%> (+<0.01%) ⬆️

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 36a3c4b...d34f57c. Read the comment docs.

@WebFreak001
Copy link
Member

want to add tests or merge as is?

@Hackerpilot
Copy link
Collaborator Author

I think that our options are

  1. Merge as-is
  2. Or
    a. merge Add an XPath-based testing system for the AST generated by the parser #425 first
    b. change the XML output for the node to actually say what kind of compile condition it is, so that the XPATH can check it
    c. write a test

@Hackerpilot
Copy link
Collaborator Author

I merged the AST testing system and now this has a regression test.

@dlang-bot dlang-bot merged commit 775d656 into master Aug 13, 2020
@Hackerpilot Hackerpilot deleted the issue-428 branch August 13, 2020 20:41
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.

ConditionalDeclaration.trueStyle not set correctly for debug conditions
3 participants