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(rosetta): diagnostics from infused snippets were not ignored #3282

Merged
merged 5 commits into from
Dec 23, 2021

Conversation

kaizencc
Copy link
Contributor

Recently we implemented the infused metadata that turns strict=false
and loose=true for the specific snippet. However we still output the
diagnostics along with other snippets (with isError=true). One thing we
could do is make these diagnostics isError=false, but I argue that we
want to ignore these diagnostics entirely. They aren't actually diagnostics;
we will find them in the cache and they do compile.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kaizencc kaizencc requested a review from rix0rrr December 21, 2021 21:26
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 21, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 22, 2021

I agree with the conclusion, but I'm not sure the premise is right. There is a bug, but I think it lies somewhere else:

Recently we implemented the infused metadata that turns strict=false and loose=true for the specific snippet. However we still output the diagnostics along with other snippets (with isError=true).

AFAIK setting strict=false should prevent isError from turning true, no? Why is that not working?

I argue that we want to ignore these diagnostics entirely. They aren't actually diagnostics; we will find them in the cache and they do compile.

The diagnostics must originate from trying to compile the snippet. So my counter-question to this observation would be: if we find them in the cache, then why are we trying to compile them in the first place?

@mergify
Copy link
Contributor

mergify bot commented Dec 23, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Dec 23, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 23, 2021

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Dec 23, 2021

Merging (with squash)...

@mergify mergify bot merged commit ad7f6a4 into main Dec 23, 2021
@mergify mergify bot deleted the conroy/compilerdiagnostics branch December 23, 2021 14:14
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants