-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Do not print error codes by default #694
Conversation
@@ -95,14 +95,20 @@ module ts { | |||
} | |||
|
|||
var category = DiagnosticCategory[diagnostic.category].toLowerCase(); | |||
output += category + " TS" + diagnostic.code + ": " + diagnostic.messageText + sys.newLine; | |||
output += category; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this should be taken care of by the flag as well. I don't think the user cares whether something is a warning or an error unless they switch this on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. By default you should simply see the error message like before the change. With the flag we should add error TSxxxx:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. this is a compromise :D
MSBuild needs the category at least. this way we lose the code, but keep the category, and still get MSBuild to work without having to pass in flags, and would be safe to checkin now, instead of having to do some release gymnastics to get it in the right branch at the right time.
If you feel strongly about it I can change it. I am fine either ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do feel strongly about it. :-) I'd rather wait for a full fix that removes the entire error TSxxxx:
string and gets us back to the concise reporting we were doing before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK then. I will hold off on this one until 1.1 ships then will push it.
Make sure to update the harness appropriately and perform a |
what will be the look of the error codes? |
What's the motivation for this change? |
👍 |
@yuit here is how it looks:
@CyrusNajmabadi, it takes space from the error message, and the information it provides is not useful, specially that we do not have any online documentation to pack these codes. |
@mhegazy "and the information it provides is not useful". I'm not sure we can make that determination without a lot more investigation on the matter. All our other compilers provide this info. Furthermore, without it, if someone is using another language, then it will be much harder to track down information about that error. It will force people to have to find out that this compiler option even exists in the first place. I think i'm ok with this change as opt-in, instead of opt-out. i.e. you get errors by default. If you don't like them, you ask for them to be disabled. if you want this feature on by default, i think we need to discuss this with the other language teams to come up with a consistent position across all our compilers. |
Agreed with Cyrus that we should find consistency with other compiler teams, but still feel that we should turn off error codes by default |
@CyrusNajmabadi "All our other compilers provide this info." Yes, and it is equally useless! It just doesn't make sense to start every diagnostic with 14 characters of meaningless fluff that pushes the interesting information to the right and causes more lines to wrap. In the "error TSxxxx: " prefix "error" is redundant (all we have is errors) and "xxxx" is some random number we don't document anywhere (and don't promise to keep in future versions). It should be off by default. |
I agree with CyrusNajmabadi. You can just search TS5007 and find a solution in any language, or use the error message and be limited to this language in search result. This was also a good idea to use "TS" prefix for the error code. It's like git, when it have been translated to french (only one year ago), it was hard to find help just by using the localized error message. Even if TSxxxx is not documented, it helps to find other users with the same problem. I don't think that a beginner will think to enable an option to view error code. But I agree that the text "error " is useless. |
ahejlsberg: I don't think the error keyword is redundant. It makes the console output more clear when for example the build process is very complex, and TypeScript compilation is only a small part of the it. For example, someone might confuse those messages with warnings or simply as some harmless debug information when quickly glancing through the long logfile. |
Adds a new compiler flag to emit error codes, and disable it by default. Fixes #693.