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

improvement: don't fail on shortType #7148

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Jan 22, 2025

resolves: #7091

I added PcQueryContext class, so it can be implicitly passed between pc methods. This way we can easily create and error report with all the context information from any place in the presentation compiler.

}
} catch {
case NonFatal(e) =>
logger.warning(s"Failed to shorten type $longType: $e")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe wrap the exception with that info instead? that way, the stack trace will include both this message and everything else it normally does. Maybe even include the history parameter in there if that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe wrap the exception with that info instead?

Are suggesting to add the info and rethrow?

One thing that I don't understand is how that error even surfaced like this. Errors inside of pc are caught and reported as error reports. What version of Scala was that exactly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are suggesting to add the info and rethrow?

yep

What version of Scala was that exactly?

I don't even remember, but it could even have been a cross-compiled project with sbt-projectmatrix (meaning all build variants would have their bloop project created, and loaded into Metals) 😬

next time I hit this (or something similar), I'll make sure to gather more info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to throw here. If it throws, you'll get no competitions, and this is just shortening the type, so we can use the long type as fallback. But I will create a full error report here instead of just logging to get more info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks

@kasiaMarek kasiaMarek requested a review from tgodzik January 23, 2025 15:45
@tgodzik tgodzik marked this pull request as ready for review January 23, 2025 16:02
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM

@tgodzik tgodzik merged commit 3714cae into scalameta:main Jan 23, 2025
21 of 23 checks passed
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.

AssertionFailed: assertion failed: (2, 1)
3 participants