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

denum.d: enable syntax highlighting of error messages #6803

Merged
merged 1 commit into from
Jun 10, 2017

Conversation

WalterBright
Copy link
Member

No description provided.

@@ -1,7 +1,7 @@
/*
TEST_OUTPUT:
---
fail_compilation/fail109.d(12): Error: enum member fail109.Bool.Unknown initialization with (Bool.True + 1) causes overflow for type 'bool'
fail_compilation/fail109.d(12): Error: enum member fail109.Bool.Unknown initialization with `Bool.True+1` causes overflow for type `bool`
Copy link
Contributor

Choose a reason for hiding this comment

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

If D had error message codes, the test could just check for the correct code instead of an exact output string... it'd make changing these more robust since you could actually rely on the test and not be changing them so frequently.

We really should add them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had error codes in DMC++. It was a giant pain in other ways.

Copy link
Member

Choose a reason for hiding this comment

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

This probably belongs on the NG, but your comment made me curious. Would you mind elaborating a bit about the troubles resulting from using error codes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does belong in the n.g. not here. Please ask there.

@WalterBright WalterBright force-pushed the denum-errors branch 4 times, most recently from 36d2435 to 83d21fe Compare May 17, 2017 20:36
@codecov-io
Copy link

Codecov Report

Merging #6803 into master will decrease coverage by 2.39%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6803     +/-   ##
=========================================
- Coverage   85.36%   82.97%   -2.4%     
=========================================
  Files         120      120             
  Lines       65807    65807             
=========================================
- Hits        56177    54601   -1576     
- Misses       9630    11206   +1576
Impacted Files Coverage Δ
src/ddmd/denum.d 86.37% <22.22%> (-8.12%) ⬇️
src/ddmd/dversion.d 53.62% <0%> (-46.38%) ⬇️
src/ddmd/tokens.d 58.33% <0%> (-39.59%) ⬇️
src/ddmd/escape.d 72.82% <0%> (-24.14%) ⬇️
src/ddmd/entity.d 83.33% <0%> (-16.67%) ⬇️
src/ddmd/safe.d 88.4% <0%> (-10.15%) ⬇️
src/ddmd/aliasthis.d 82.19% <0%> (-9.59%) ⬇️
src/ddmd/sideeffect.d 88.23% <0%> (-7.06%) ⬇️
src/ddmd/lexer.d 82.11% <0%> (-7%) ⬇️
src/ddmd/attrib.d 82.78% <0%> (-6.73%) ⬇️
... and 46 more

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 50d5eba...83d21fe. Read the comment docs.

@yebblies
Copy link
Member

Why was it necessary to change error messages and refactor the test files? I'd be much more comfortable merging this if it wasn't 90% unrelated changes.

@WalterBright
Copy link
Member Author

Why was it necessary to change error messages

The syntax highlighting looks for text between backticks. This has become a de-facto standard in markup, and is used by Ddoc as well.

and refactor the test files?

I merged a couple of them to reduce the number of files. The tests are the same.

@yebblies
Copy link
Member

The syntax highlighting looks for text between backticks. This has become a de-facto standard in markup, and is used by Ddoc as well.

No I get that, but you also made unrelated edits to error messages for some reason.

I merged a couple of them to reduce the number of files. The tests are the same.

Yeah, I get that too, but it didn't need to be in this PR and I assume is behind the codecov spam. TBH I think the tradeoff is different with the fail_compilation tests and the would be better off in multiple files instead of combined together. It's also nice to have the bugzilla number and filename match, instead of only having that information in the comments.

Also, jenkins is failing again.

@WalterBright
Copy link
Member Author

but it didn't need to be in this PR

If I didn't do it in this PR it would never get done, because I wouldn't remember which files of the pile of them could be merged later.

TBH I think the tradeoff is different with the fail_compilation tests and the would be better off in multiple files instead of combined together.

It would have been less work for me if they were grouped together more.

It's also nice to have the bugzilla number and filename match, instead of only having that information in the comments.

grep NNNN * is more reliable and not any harder than ls *NNNN*

@WalterBright
Copy link
Member Author

jenkins is failing again.

Working now.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

LGTM, @yebblies let me know if there's any blocker.

@dlang-bot dlang-bot merged commit f1ecc9d into dlang:master Jun 10, 2017
@WalterBright WalterBright deleted the denum-errors branch June 10, 2017 03:38
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.

7 participants