Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: allow configuration through workspace/didChangeConfiguration#316

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

willruggiano
Copy link
Contributor

What kind of change does this PR introduce?

It enables passing workspace settings via the workspace/didChangeConfiguration notification. This in turn enables the client to specify settings dynamically, rather than being limited to configuration files. This isa solution to#302. See example usage (with lspconfig) here:willruggiano/neovim.drv@9aa06ad.

What is the current behavior?

There is none. The payload of this handler is currently ignored.

What is the new behavior?

The configuration received by the handler is merged with the fs configuration.

@willruggiano
Copy link
ContributorAuthor

willruggiano commentedApr 5, 2025•
edited
Loading

This is literally my first time writing Rust, ever. lmao.

Furthermore, I started this in draft because I'm not sure what you want with regards to testing and such. Is it safe to change workspace settings at runtime? No idea. Is my code nasty? Certainly.

Anyways, let me know how/if you'd like to proceed with this. I thought it would be pretty straightforward after thinking it through while reading#302 and, I don't know, kinda felt the random urge to finally give Rust a try :)

Copy link
Collaborator

@juleswritescodejuleswritescode left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Hey hey! 👋

First of all, thank you for the contribution :) And sorry for the late reply.

The approach looks good to me; I first thought that it would be nicer to use aupdate_settings method for this, but I think it's fine as it is.

I have a few remarks regarding code, but great first Rust PR 👍

@willruggianowillruggiano marked this pull request as ready for reviewApril 23, 2025 05:38
@willruggiano
Copy link
ContributorAuthor

willruggiano commentedApr 23, 2025•
edited
Loading

@juleswritescode166f5aa addresses your comments, or at least attempts to :D

Copy link
Collaborator

@juleswritescodejuleswritescode left a comment

Choose a reason for hiding this comment

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

I think it looks good 👍🏻👍🏻

@juleswritescode
Copy link
Collaborator

juleswritescode commentedApr 25, 2025•
edited
Loading

Heads up, don't worry about the linting issues,this pr fixes that.

Once that's merged, could you runjust lint-ci locally?

and runjust lint-ci-versions before that, the output should look like this:

Screenshot 2025-04-25 at 12 50 42
willruggiano reacted with thumbs up emoji

@willruggianowillruggianoforce-pushed thefeat/didChangeConfiguration branch from166f5aa to550049eCompareApril 25, 2025 21:05
@@ -1,5 +1,5 @@
{
"$schema": "https://biomejs.dev/schemas/1.9.4/schema.json",
"$schema": "./node_modules/@biomejs/biome/configuration_schema.json",
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

so you don't have to update the schema url if you upgrade biome

Copy link
Collaborator

Choose a reason for hiding this comment

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

hella smart!

Copy link
Collaborator

Choose a reason for hiding this comment

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

does this work if we are using biome via egpnpx?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It does not. Nor will it work withbunx (ornpx for that matter).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of pointing to the latest remote schema?

But no hard feelings. 👍🏼

Copy link
ContributorAuthor

@willruggianowillruggianoApr 30, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I think you're better off pointing at a pinned remote schema. Want me to revert this change?

@willruggiano
Copy link
ContributorAuthor

@juleswritescode I made a minor change to your biome.jsonc, see inline comments

juleswritescode and psteinroe reacted with rocket emoji

Copy link
Collaborator

@psteinroepsteinroe left a comment

Choose a reason for hiding this comment

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

@juleswritescode lets merge? :)

juleswritescode reacted with thumbs up emoji
@juleswritescode
Copy link
Collaborator

@psteinroe Yeah please go ahead, I wasn't really in the loop regarding thebiome.jsonc changes

@psteinroepsteinroe merged commit26e82c7 intosupabase-community:mainMay 7, 2025
7 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@juleswritescodejuleswritescodejuleswritescode approved these changes

@psteinroepsteinroepsteinroe approved these changes

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

Successfully merging this pull request may close these issues.

3 participants
@willruggiano@juleswritescode@psteinroe
[8]ページ先頭
©2009-2025 Movatter.jp