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

[7.0] Remove quotes option #5154

Merged
merged 2 commits into from Jan 31, 2017
Merged

[7.0] Remove quotes option #5154

merged 2 commits into from Jan 31, 2017

Conversation

@mswiecicki
Copy link
Contributor

@mswiecicki mswiecicki commented Jan 18, 2017

Q A
Patch: Bug Fix?
Major: Breaking Change? hazoo said so in the issue
Minor: New Feature?
Deprecations? quotes option in babel-generator doesn't exist anymore, so yes
Spec Compliancy? not sure... ?
Tests Added/Pass? none added but all pass
Fixed Tickets closes #5139 and #4803
License MIT
Doc PR
Dependency Changes none

All details in commit messages (probably more than is really needed)

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Jan 18, 2017

Hey @4rlekin! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 19, 2017

Codecov Report

Merging #5154 into 7.0 will increase coverage by -0.01%.

@@            Coverage Diff             @@
##              7.0    #5154      +/-   ##
==========================================
- Coverage   89.61%   89.61%   -0.01%     
==========================================
  Files         200      200              
  Lines        9753     9752       -1     
  Branches     2597     2596       -1     
==========================================
- Hits         8740     8739       -1     
  Misses       1013     1013
Impacted Files Coverage Δ
packages/babel-generator/src/index.js 90.56% <ø> (-0.18%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e530e3c...2e5975c. Read the comment docs.

@hzoo hzoo changed the title Remove quotes option [7.0] Remove quotes option Jan 20, 2017
@@ -58,7 +58,6 @@ function normalizeOptions(code, opts, tokens): Format {
compact: opts.compact,
minified: opts.minified,
concise: opts.concise,
quotes: opts.quotes || findCommonStringDelimiter(code, tokens),

This comment has been minimized.

@hzoo

hzoo Jan 20, 2017
Member

Do we want to keep findCommonStringDelimiter? If we do we then this pr can just be removing opts.quotes rather than defaulting to double.

I wasn't sure at the time

This comment has been minimized.

@existentialism

existentialism Jan 20, 2017
Member

Yeah, I guess we never really decided if we wanted to default to double or infer. The more I think about it, I think infer is better option?

This comment has been minimized.

@hzoo

hzoo Jan 28, 2017
Member

@4rlekin Can we just remove the top level quotes option? Basically I think we can just do quotes: findCommonStringDelimiter(code, tokens), so that it still runs findCommonStringDelimiter. Basically we just need to not allow the user to specify the quotes and rather we infer it.

@mswiecicki
Copy link
Contributor Author

@mswiecicki mswiecicki commented Jan 31, 2017

Provided Travis build will go without issue it should be all good now, sorry for the delay.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Jan 31, 2017

Hey @4rlekin! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@mswiecicki
Copy link
Contributor Author

@mswiecicki mswiecicki commented Jan 31, 2017

Hmm... i swear to god i re-generated those exact tests, dunno why it fails, could someone check this out ?
Or explain what happened ? When i ran tests it was all good...

@hzoo
Copy link
Member

@hzoo hzoo commented Jan 31, 2017

yeah I'l check out

@hzoo
Copy link
Member

@hzoo hzoo commented Jan 31, 2017

I think you can just remove the last commit and it should be good because tests are the same as before (with the new change of just removing the option)

@hzoo hzoo merged commit ba0df23 into babel:7.0 Jan 31, 2017
2 of 3 checks passed
2 of 3 checks passed
codecov/project 89.61% (-0.01%) compared to e530e3c
Details
codecov/patch Coverage not affected when comparing e530e3c...2e5975c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo
Copy link
Member

@hzoo hzoo commented Jan 31, 2017

Thanks @4rlekin I just rebased and removed the test fixture changes since they weren't needed anymore

hulkish added a commit to hulkish/babel that referenced this pull request May 2, 2017
* 7.0: (37 commits)
  resolved conflicts
  [7.0] Switch decorators-legacy to decorators in the Stage 1 Preset (babel#5318) (babel#5319)
  [7.0] Replacing current decorators with decorators-legacy (babel#5290)
  Add Node 7 to CI (babel#5165)
  [7.0] remove standalone babel package (babel#5293)
  .gitignore for test [skip ci]
  update yarn
  use lerna@2-beta.37 (babel#5254)
  [7.0] Run Babel's unittests in a custom sandbox (take 2). (babel#5263)
  [7.0] Remove quotes option (babel#5154)
  [7.0] List babylon plugins instead of * in babel-generator tests (babel#5231)
  Remove babel-runtime from packages' dependencies (babel#5218)
  Bump `detect-indent`. (babel#5226)
  [7.0] Add legacy-decorators to stage-1. Fixes babel#5220 (babel#5225)
  [7.0] Use lerna's --independent mode + changes (fixes babel#5221)
  [7.0] Bump `home-or-tmp` for `babel-register`. (babel#5189)
  [7.0] Added yarn.lock (babel#5175)
  [7.0] Remove old babel-runtime code (babel#5187)
  [7.0] Drop support for Node 5 (babel#5186)
  Remove path-is-absolute in favor of builtin path.isAbsolute (babel#5179)
  ...
@lock lock bot added the outdated label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.