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

[change] pass originId into server responses #289

Merged
merged 2 commits into from Aug 23, 2022

Conversation

azdrojowa123
Copy link
Contributor

Additionally I added BazelFlag.color(true) for building target (useful in intellij-bsp plugin).

Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

im not sure about passing the origin id to the project storage, but it seems useful since we are logging there as well - @lukaszwawrzyk what do u think?

@lukaszwawrzyk
Copy link
Contributor

im not sure about passing the origin id to the project storage, but it seems useful since we are logging there as well - @lukaszwawrzyk what do u think?

We are loging it because automagic error reporting is broken in IntelliJ duriing build, but not during sync. Maybe new bsp plugin could fix that. Maybe we can keep ProjectStorage as it was before. The api is confusing now, Initially I though that project is cached by origin id now or something and it is just error logging.

Additionally I added BazelFlag.color(true) for building target (useful in intellij-bsp plugin).

Sadly it breaks the original scala plugin with bsp support as its build console doesn't support colors :/

To solve this we might check who is our bsp client, and if it is the new plugin we can color the output.


Another thing. There is a lot of context passing (originId) that makes it harder to see what is important. It should be possible to refactor. Not sure if it can be perfect, but some places would be more clean.
How about adding a method on BspClientLogger like withOrigin(originId). It would return a new logger object which logs everything under this origin id, so we don't have to pass it to each method call anymore. You could just pass such adjusted logger in some places instead of passing in origin id through couple of methods so that it can be used in the logger call. I think this idea might be taken further, but it might be good enough.

@azdrojowa123 azdrojowa123 force-pushed the repair-originId branch 2 times, most recently from 935a6cc to e82c2a2 Compare August 22, 2022 08:06
@abrams27 abrams27 changed the title Pass originId into server responses [change] Pass originId into server responses Aug 23, 2022
@abrams27 abrams27 changed the title [change] Pass originId into server responses [change] pass originId into server responses Aug 23, 2022
@abrams27 abrams27 merged commit 6ab5f37 into JetBrains:master Aug 23, 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.

None yet

3 participants