Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

Add config to disable default formatter #10

Merged
merged 7 commits into from Sep 19, 2019

Conversation

domoritz
Copy link
Contributor

@domoritz domoritz commented Aug 2, 2019

@domoritz
Copy link
Contributor Author

@aeschli is this what you imagined?

@aeschli
Copy link
Contributor

aeschli commented Aug 16, 2019

Having it as part of DiagnosticsOptions is strange.

Maybe a Capabilities property on the LanguageServiceDefaults? Also there needs to be code that listens to the change event and register/unregister the formatter.

@domoritz domoritz changed the title add config to disable default formatter Add config to disable default formatter Aug 17, 2019
@domoritz
Copy link
Contributor Author

Thank you for taking a look at my PR. I updated it to follow your suggestions.

@aeschli
Copy link
Contributor

aeschli commented Aug 19, 2019

Thanks, improvements are good.
I would still suggest to have a new settings options Capabilities and have 'formatter` in there, so we can use that object also for other requests to not register some features.

Also, now onDidChange is fired, but isn't it necessary to listen to it and register/deregister the services?

@domoritz
Copy link
Contributor Author

Oh, I misunderstood your comment and didn't read Capabilities as Capabilities. I'll work on this.

I had assumed that this._onDidChange.fire(this) would reload the services. I guess I have to keep a reference to the services/providers so that I can deregister them, right?

@domoritz
Copy link
Contributor Author

What are the disposables for, btw? The array doesn't seem to be used anywhere.

@domoritz
Copy link
Contributor Author

Done. Let me know if this is what you imagined.

Btw, I noticed that the indentation is inconsistent (space vs tabs). What's the preferred indentation?

@aeschli
Copy link
Contributor

aeschli commented Aug 19, 2019

looks good!
should be tabs.
yes, there's currently no way to uninstall the language supports, so the disposables are not used.

@domoritz
Copy link
Contributor Author

I can remove disposables if you want or just leave it. Other than that, I'm done with the PR.

@aeschli
Copy link
Contributor

aeschli commented Aug 19, 2019

Thanks, I'll take it from here!

@domoritz
Copy link
Contributor Author

Thank you. Monaco is really awesome and especially the JSON support has been phenomenal for the editor we are building (https://vega.github.io/editor/).

@aeschli aeschli self-requested a review August 20, 2019 14:32
src/monaco.d.ts Outdated
/**
* Disable the default JSON formatter.
* Defines wheter the built-in documentFormattingEdit provider is enabled.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo. Should be "whether".

src/monaco.d.ts Outdated
readonly documentFormattingEdits?: boolean;

/**
* Defines wheter the built-in documentRangeFormattingEdit provider is enabled.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same typo as above (also note the same typo below)

@aeschli aeschli self-assigned this Aug 21, 2019
@aeschli aeschli requested a review from alexdima August 21, 2019 08:14
@aeschli
Copy link
Contributor

aeschli commented Aug 21, 2019

@alexandrudima Here's what we ended up with the APIs to enable/disable certain providers in JSON.
Let me know if that makes sense for you.

@domoritz
Copy link
Contributor Author

Any updates on this @aeschli @alexandrudima?

@alexdima alexdima merged commit 58c8efe into microsoft:master Sep 19, 2019
@alexdima alexdima added this to the August 2019 milestone Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants