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

Parsing Error messages refactor #331

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

johnny-keker
Copy link
Contributor

@johnny-keker johnny-keker commented Sep 12, 2022

This PR provides some code refactor for Error messages parsing logic.

Notable change: we do not handle error formats in which location are mentioned after error text.

VSRAD.Package/BuildTools/Errors/Parser.cs Outdated Show resolved Hide resolved
VSRAD.Package/BuildTools/Errors/Parser.cs Outdated Show resolved Hide resolved
VSRAD.Package/BuildTools/Errors/Parser.cs Outdated Show resolved Hide resolved
@timlathy
Copy link
Contributor

Good news: with the latest changes to our tooling, messages like *W,code: text (C:\Absolute\Path\source.c:3, C:\Absolute\Path\source.c:5, C:\Absolute\Path\include.h:10) will no longer be generated, so there's no need to handle this format.

I would suggest keeping most of the changes in this PR with the exception of the code that handles locations after error text (it can be safely removed). I would also prefer to go back to the previous signature of Parse... methods (have them return Message or null). I think it's a good style to keep functions pure by avoiding in-out parameters where it's possible.

@johnny-keker
Copy link
Contributor Author

Good news: with the latest changes to our tooling, messages like *W,code: text (C:\Absolute\Path\source.c:3, C:\Absolute\Path\source.c:5, C:\Absolute\Path\include.h:10) will no longer be generated, so there's no need to handle this format.

I would suggest keeping most of the changes in this PR with the exception of the code that handles locations after error text (it can be safely removed). I would also prefer to go back to the previous signature of Parse... methods (have them return Message or null). I think it's a good style to keep functions pure by avoiding in-out parameters where it's possible.

Thanks! Is 9fd8cb2 works for you?

@johnny-keker johnny-keker changed the title Error List multiple locations errors fix Parsing Error messages refactor Sep 15, 2022
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.

3 participants