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.

Sign up for GitHub

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

Jump to bottom

fix: do not expose mutable env properties in State #2016

Merged
merged 1 commit into from
Mar 10, 2023
Merged

fix: do not expose mutable env properties in State #2016

merged 1 commit into from
Mar 10, 2023
+39 −33

Conversation

kpodsiad
Copy link
Collaborator

@kpodsiad kpodsiad commented Mar 9, 2023

fixes scalameta/metals#5025

  • change mutable PrettyProperties to wrapper over immutable Map
  • adjust forker's implementations logic so they are not even aware of env parameters. They're handled one step before

I was thinking about some tests but immutable data structure should be just enough to prevent such cases in the future

@kpodsiad kpodsiad requested review from tgodzik and dos65 March 9, 2023 20:15
@kpodsiad
Copy link
Collaborator Author

Debug tests seems to be flaky as they passed on few checks and failed on 2

ckipp01
ckipp01 approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

👍🏼 LGTM

And yes, the failing tests have been super flaky lately.

@kpodsiad kpodsiad merged commit 3d9d785 into main Mar 10, 2023
@kpodsiad kpodsiad deleted the kpodsiadlo/fix-state-env branch March 10, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckipp01 ckipp01

@tgodzik tgodzik

@dos65 dos65

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metals (or vscode) reuse of the debug console process mix-up env
2 participants
@kpodsiad @ckipp01