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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce option value via types #1302

Merged
merged 2 commits into from
Feb 28, 2018

Conversation

bmf-ribeiro
Copy link
Contributor

Closes #889

If I missed some property, or if I should do something else, feel free to say something! 馃槃

@Tyriar
Copy link
Member

Tyriar commented Feb 26, 2018

The proposed change actually makes it worse I think as the typescript server will then present "BellStyle" as the type instead of showing all the strings which is more useful.

I think what I was getting at for #889 was to add more specific setOption calls, which is how the string values are already:

setOption(key: 'fontWeight' | 'fontWeightBold', value: null | 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900'): void;

@bmf-ribeiro
Copy link
Contributor Author

Yes, that makes complete sense.

Then, the proper fix was to remove the overloads with type any, as well as adding the missing call, correct?

@Tyriar
Copy link
Member

Tyriar commented Feb 27, 2018

@nikonso the fix happened a while ago, back when there was only a single setOption for 'string' value types.

@bmf-ribeiro
Copy link
Contributor Author

Yes but doesn't this pull request still make sense nevertheless as is?

Because of the setOption overload with the type any the typings-test.ts could miss some cases, and it was by removing it that I found that the set option overload for rows and cols was missing..

Otherwise, feel free to close it 馃槈

@Tyriar
Copy link
Member

Tyriar commented Feb 27, 2018

Oh I missed your new commit, the rows/cols thing is definitely good 馃憤

I think I tried to remove getOption(key: string): any before but then I couldn't call it with generic string object as it was too strict. For example, I think we want to allow this:

const someString: string = 'foo';
term.getOption(someString)

When I tried a while ago this did not work, which would be a bad dev experience if you need to cast it to <'rows'>someString.

@Tyriar
Copy link
Member

Tyriar commented Feb 28, 2018

Thanks again @nikonso! 馃槂

@Tyriar Tyriar merged commit 131e6ff into xtermjs:master Feb 28, 2018
@Tyriar Tyriar added this to the 3.2.0 milestone Feb 28, 2018
@Tyriar Tyriar self-assigned this Feb 28, 2018
@bmf-ribeiro bmf-ribeiro deleted the Enforce-option-value-via-types branch February 28, 2018 21:50
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

2 participants