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: CJSImportTransformer#processAssignment #402

Merged
merged 3 commits into from
Jan 12, 2019

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Jan 11, 2019

Closes #401

In the presence of a type token, there's nothing to do.

Fixes alangpierce#401
@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #402 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #402   +/-   ##
======================================
  Coverage    79.9%   79.9%           
======================================
  Files          45      45           
  Lines        4951    4951           
  Branches     1152    1152           
======================================
  Hits         3956    3956           
  Misses        698     698           
  Partials      297     297
Impacted Files Coverage Δ
src/transformers/CJSImportTransformer.ts 92.18% <100%> (ø) ⬆️

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 8bc57a1...bdfbd98. Read the comment docs.

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Thank you! A few little docs improvements that I put in as suggestions, but looks good to me!

(I'll see if I can just apply the suggestions myself and merge if the build passes afterward.)

src/transformers/CJSImportTransformer.ts Show resolved Hide resolved
test/typescript-test.ts Outdated Show resolved Hide resolved
@alangpierce alangpierce self-assigned this Jan 12, 2019
@alangpierce
Copy link
Owner

@aleclarson drat, almost worked (I think this came up in a previous PR as well). Would you mind accepting the suggestions? (Also happy to discuss tweaks.)

screen shot 2019-01-12 at 10 15 26 am

@aleclarson
Copy link
Contributor Author

Good to go 👍

@alangpierce
Copy link
Owner

Hmm something's up with the travis config, I'll see if I can get that working before merging... there does seem to be a build going, though https://travis-ci.org/alangpierce/sucrase/builds/478806484 , but it's not showing up here.

@alangpierce
Copy link
Owner

alangpierce commented Jan 12, 2019

Ah, looks like maybe the github services deprecation means I need to set it up again...

https://developer.github.com/changes/2018-04-25-github-services-deprecation/ https://developer.github.com/changes/2018-11-05-github-services-brownout/

That's annoying. Anyway, it still triggered a build, which passed, so this should be good to go!

(Edit: nevermind, it did show up as a check... not sure why it didn't before.)

@alangpierce alangpierce merged commit b7b9ad1 into alangpierce:master Jan 12, 2019
@aleclarson
Copy link
Contributor Author

Have you tried semantic-release before? I use it on all of my repos nowadays. 😄

@alangpierce
Copy link
Owner

Yep, I have used it before! I was meaning to get something like that set up for Sucrase, but after the eslint-scope fiasco ( https://eslint.org/blog/2018/07/postmortem-for-malicious-package-publishes ), I decided that I need two-factor auth for npm publishes (auth-and-writes), which doesn't work with semantic-release ( https://github.com/semantic-release/npm#npm-registry-authentication ) or any system that publishes from CI.

I also like having a changelog that's a little more human-curated rather than just all of the commit message titles.

I am certainly on-board with frequent publishes. I'll do a publish pretty soon, definitely this weekend, I just want to see if I can get a few more fixes in.

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.

3 participants