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

fix(39332): QuoteStyle not respected for string literal types in implement interface #39348

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #39332

@minestarks
Copy link
Member

@jessetrinity could you review please?

Copy link
Contributor

@jessetrinity jessetrinity left a comment

Choose a reason for hiding this comment

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

Should it respect "auto" quotePreference? It does seem unfortunate that we use the same quotePreference everywhere but the auto behavior is only determined from pre-existing imports.

@a-tarasyuk
Copy link
Contributor Author

@jessetrinity Good question. Which result should be generated for the following code with the auto option?

interface I {
   a(): void;
   b(x: 'x', y: 'a' | 'b'): 'b';

   c: 'c';
   d: { e: 'e'; };
}

class Foo/**/ implements I {
}

@jessetrinity
Copy link
Contributor

@a-tarasyuk If auto were respected I would expect the output to be based on the first use of quotes in an interface member. In this example, single quotes since you have b(x: 'x', y: 'a' | 'b'): 'b';, or double quotes if it were instead b(x: "x", y: "a" | "b"): "b";.

I'm not sure if that decision makes sense though since I think auto might only apply to imports?

@a-tarasyuk
Copy link
Contributor Author

@jessetrinity Do you mean we need to find the use of the first line in the file to determine the quotePreference before generating the code?

@jessetrinity
Copy link
Contributor

@a-tarasyuk that is the usual way of doing it. I think getQuotePreference in utilities is what you want.

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Jul 2, 2020

@jessetrinity I checked this helper, it seems to detect quotePreference based on imports. If it is ok, I'll use it to get a quote option.

@jessetrinity
Copy link
Contributor

@a-tarasyuk That seems okay. It looks like the setting is worded to only work based on imports in editors anyway.

@a-tarasyuk
Copy link
Contributor Author

@jessetrinity Ok, changed and added several additional tests for auto option.

Copy link
Contributor

@jessetrinity jessetrinity left a comment

Choose a reason for hiding this comment

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

Found a typo(?) but otherwise looks great!

tests/cases/fourslash/fourslash.ts Outdated Show resolved Hide resolved
@jessetrinity
Copy link
Contributor

I think this commit will fix the eslint failures:
#39450

@jessetrinity jessetrinity merged commit 2a92a6e into microsoft:master Jul 7, 2020
weswigham added a commit to weswigham/TypeScript that referenced this pull request Jul 7, 2020
cangSDARM added a commit to cangSDARM/TypeScript that referenced this pull request Jul 9, 2020
* upstream/master: (75 commits)
  Insert auto-imports in sorted order (microsoft#39394)
  LEGO: check in for master to temporary branch.
  Better checking of @param/@Property tags (microsoft#39487)
  fix(25155): add space before optional parameters/properties (microsoft#38798)
  Add regression test for microsoft#38834 (microsoft#39479)
  Fixes searches for symbols exported using export * as (microsoft#39507)
  fix(39421): omit prefix text for rest binding element (microsoft#39433)
  fix(39440): show QF for abstract classes with methods which include 'this' parameter (microsoft#39465)
  Remove unnecessary assert (microsoft#39483)
  LEGO: check in for master to temporary branch.
  Update user baselines (microsoft#39220)
  Type `this` in more constructor functions (microsoft#39447)
  LEGO: check in for master to temporary branch.
  LEGO: check in for master to temporary branch.
  Properly handle rest parameters in function declarations with @type annotations (microsoft#39473)
  Ensure type/namespaceish statics are included in the list of namespace merge members (microsoft#38920)
  Fix getTypeAtLocation for dotted implements clauses (microsoft#39363)
  Add workflow_dispatch to our nightly publish script. (microsoft#39485)
  Fix crash in decorator metadata calculation when serializing template literal type nodes (microsoft#39481)
  Fix test semantic merge conflict between microsoft#39348 and microsoft#39130 (microsoft#39478)
  ...

# Conflicts:
#	src/compiler/scanner.ts
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.

QuoteStyle not respected for string literal types in implement interface
4 participants