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] Replacing current decorators #5290

Merged
merged 9 commits into from Feb 15, 2017

Conversation

Projects
None yet
7 participants
@alxpy
Contributor

alxpy commented Feb 9, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? yes
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets Close #5283
License MIT
Doc PR
Dependency Changes

I started working on #5283 and I have some doubts about the tests. Please look at the tests structure. If all is well, I move other tests.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 9, 2017

@alxpy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @kaicataldo and @raspo to be potential reviewers.

@alxpy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @kaicataldo and @raspo to be potential reviewers.

@hzoo hzoo added this to the Babel 7 milestone Feb 9, 2017

hzoo added a commit that referenced this pull request Feb 10, 2017

hzoo added a commit that referenced this pull request Feb 10, 2017

@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 11, 2017

Contributor

@hzoo So? Did I make everything right? Can I continue?

Contributor

alxpy commented Feb 11, 2017

@hzoo So? Did I make everything right? Can I continue?

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Feb 12, 2017

Member

I updated the README to be consistent with the other packages.

Member

xtuc commented Feb 12, 2017

I updated the README to be consistent with the other packages.

@@ -46,24 +46,42 @@ function enumerable(value) {
}
```
-## Installation
+## Installation & Usage

This comment has been minimized.

@existentialism

existentialism Feb 12, 2017

Member

Nit: to keep it consistent with the rest of the docs let's keep Installation and Usage as separate sections

@existentialism

existentialism Feb 12, 2017

Member

Nit: to keep it consistent with the rest of the docs let's keep Installation and Usage as separate sections

```sh
-npm install --save-dev babel-plugin-transform-decorators
+npm install --save-dev babel-plugin-transform-decorators`

This comment has been minimized.

@xtuc

xtuc Feb 12, 2017

Member

Oops, I added a trailing backtick there.

@xtuc

xtuc Feb 12, 2017

Member

Oops, I added a trailing backtick there.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Feb 14, 2017

Collaborator

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

Collaborator

babel-bot commented Feb 14, 2017

Hey @alxpy! 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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 14, 2017

Codecov Report

Merging #5290 into 7.0 will increase coverage by 0.47%.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##              7.0    #5290      +/-   ##
==========================================
+ Coverage   89.88%   90.35%   +0.47%     
==========================================
  Files         200      200              
  Lines        9884     9931      +47     
  Branches     2668     2698      +30     
==========================================
+ Hits         8884     8973      +89     
+ Misses       1000      958      -42
Impacted Files Coverage Δ
...ges/babel-plugin-transform-decorators/src/index.js 94.28% <94.23%> (+77.04%)

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 c3098d2...488a064. Read the comment docs.

codecov-io commented Feb 14, 2017

Codecov Report

Merging #5290 into 7.0 will increase coverage by 0.47%.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##              7.0    #5290      +/-   ##
==========================================
+ Coverage   89.88%   90.35%   +0.47%     
==========================================
  Files         200      200              
  Lines        9884     9931      +47     
  Branches     2668     2698      +30     
==========================================
+ Hits         8884     8973      +89     
+ Misses       1000      958      -42
Impacted Files Coverage Δ
...ges/babel-plugin-transform-decorators/src/index.js 94.28% <94.23%> (+77.04%)

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 c3098d2...488a064. Read the comment docs.

@existentialism

This comment has been minimized.

Show comment
Hide comment
@existentialism

existentialism Feb 14, 2017

Member

Nice work @alxpy!

For the tests, it may be better if we break each test up into its own folder/exec so any specific failure would be easy to identify.

|- fixtures
|     |- class
|           |- descriptor-expr-order
|           |     |- exec.js
|           |- method-reverse-order
|           |     |- exec.js
Member

existentialism commented Feb 14, 2017

Nice work @alxpy!

For the tests, it may be better if we break each test up into its own folder/exec so any specific failure would be easy to identify.

|- fixtures
|     |- class
|           |- descriptor-expr-order
|           |     |- exec.js
|           |- method-reverse-order
|           |     |- exec.js
@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 14, 2017

Contributor

@existentialism I have tried to do like you wrote, but with this structure, expected.js was created =(

Contributor

alxpy commented Feb 14, 2017

@existentialism I have tried to do like you wrote, but with this structure, expected.js was created =(

@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 14, 2017

Contributor

@existentialism sorry, I was wrong. I forgot to remove the exec folder

Contributor

alxpy commented Feb 14, 2017

@existentialism sorry, I was wrong. I forgot to remove the exec folder

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Feb 14, 2017

Collaborator

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

Collaborator

babel-bot commented Feb 14, 2017

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

@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 14, 2017

Contributor

@existentialism so, I think I moved all tests

Contributor

alxpy commented Feb 14, 2017

@existentialism so, I think I moved all tests

+
+Wrong:
+
+```js

This comment has been minimized.

@hzoo

hzoo Feb 15, 2017

Member

Can we either not make this js or change it to json with wrapping {}?

@hzoo

hzoo Feb 15, 2017

Member

Can we either not make this js or change it to json with wrapping {}?

This comment has been minimized.

@alxpy

alxpy Feb 15, 2017

Contributor

yes

@alxpy

alxpy Feb 15, 2017

Contributor

yes

@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 15, 2017

Contributor

I finished work. Do I need to fix something?

Contributor

alxpy commented Feb 15, 2017

I finished work. Do I need to fix something?

"babel-plugin-syntax-decorators": "^6.13.0",
- "babel-helper-explode-class": "^6.22.0",
+ "babel-runtime": "^6.2.0",

This comment has been minimized.

@hzoo

hzoo Feb 15, 2017

Member

note: after this is merged, we should remove this dep in 7.0

@hzoo

hzoo Feb 15, 2017

Member

note: after this is merged, we should remove this dep in 7.0

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Feb 15, 2017

Member

Nope just a few docs things I'l do when merging, thanks!

Member

hzoo commented Feb 15, 2017

Nope just a few docs things I'l do when merging, thanks!

@hzoo hzoo merged commit fa2a373 into babel:7.0 Feb 15, 2017

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Feb 15, 2017

Member

Really nice work @alxpy! looking forward to more contributors 😄

Member

hzoo commented Feb 15, 2017

Really nice work @alxpy! looking forward to more contributors 😄

@alxpy

This comment has been minimized.

Show comment
Hide comment
@alxpy

alxpy Feb 15, 2017

Contributor

@hzoo I was glad to help! I think that I will be helping again.

Contributor

alxpy commented Feb 15, 2017

@hzoo I was glad to help! I think that I will be helping again.

@alxpy alxpy referenced this pull request in babel/website Mar 6, 2017

Merged

Fix PR number (legacy-decorators) #1189

hulkish added a commit to hulkish/babel that referenced this pull request May 2, 2017

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