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.

Sign up for GitHub

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

Jump to bottom

Import babel-plugin-transform-dotall-regex #7059

Merged
merged 1 commit into from
Dec 19, 2017
Merged

Import babel-plugin-transform-dotall-regex #7059

merged 1 commit into from
Dec 19, 2017

Conversation

mathiasbynens
Copy link
Contributor

Original: https://github.com/mathiasbynens/babel-plugin-transform-dotall-regex

Moving it into the official Babel repository makes it easier to maintain the transform.

@mathiasbynens
Copy link
Contributor Author

PTAL @thejameskyle. Feel free to edit the patch directly — especially for small nits that’s much easier/faster for everyone.

xtuc
xtuc reviewed Dec 19, 2017
View reviewed changes
Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Could you please run prettier on the code? make fix

It seems that tests are failing but i'm not sure it's related to your PR actually.

packages/babel-plugin-transform-dotall-regex/test/index.js Outdated
import path from 'path';
import fs from 'fs';
import assert from 'assert';
import { transform, transformFileSync } from '@babel/core';
Copy link
Member

Choose a reason for hiding this comment

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

transform is not used, we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mathiasbynens
Copy link
Contributor Author

@xtuc Thanks! Patch updated.

hzoo
hzoo reviewed Dec 19, 2017
View reviewed changes
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

You'll need to change the test file to match the other packages, don't need the file anymore and use the helper pkg

@mathiasbynens
Copy link
Contributor Author

@hzoo Thanks; I’ve changed test/index.js.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 19, 2017 •
edited
Loading

@mathiasbynens There are a few things that needs to be changed in the tests:

  • input.js -> actual.js
  • You should add an options.json file in fixtures' subfolders, to actually enable this plugin: 
    {
  "plugins": [ "transform-dotall-regex" ]
}
  • The tests needs to be one level deeper

You can look for a good example at https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-sticky-regex/test

@mathiasbynens
Copy link
Contributor Author

@nicolo-ribaudo Thanks for pointing to that example. I’ve updated the patch.

As a side comment, I’m struggling to understand how the actual.js file name makes sense for the input. In tests, the term “actual” usually refers to the dynamically generated output based on the input, not to the input itself.

nicolo-ribaudo
nicolo-ribaudo reviewed Dec 19, 2017
View reviewed changes
packages/babel-plugin-transform-dotall-regex/README.md Outdated
## Installation

```sh
$ npm install @babel/plugin-transform-dotall-regex
Copy link
Member

Choose a reason for hiding this comment

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

--save-dev

@nicolo-ribaudo
Copy link
Member

As a side comment, I’m struggling to understand how the actual.js file name makes sense for the input. In tests, the term “actual” usually refers to the dynamically generated output based on the input, not to the input itself.

Yeah, you are right... I think that it is some sort of tecnichal debt that no one ever cared about fixing.

PS. Still mv -t packages/babel-plugin-transform-dotall-regex/test/fixtures/dotall-regex packages/babel-plugin-transform-dotall-regex/test/fixtures/{simple,with-unicode-flag} 😉

@hzoo
Copy link
Member

hzoo commented Dec 19, 2017

Yeah we should refactor the tests to just be input/output files or something, it's always been actual/expected and it's confusing

@hzoo hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 19, 2017
@existentialism
Copy link
Member

@mathiasbynens updated the readme, and the tests

@hzoo hzoo mentioned this pull request Dec 19, 2017
Closed
@hzoo
Copy link
Member

hzoo commented Dec 19, 2017

Made #7063 @mathiasbynens

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 19, 2017 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6248/

@babel babel deleted a comment from babel-bot Dec 19, 2017
@mathiasbynens
Import babel-plugin-transform-dotall-regex
5da53d4 
Original: https://github.com/mathiasbynens/babel-plugin-transform-dotall-regex

Moving it into the official Babel repository makes it easier to maintain the transform.
@existentialism
Copy link
Member

After we land this, I have a small follow-up PR that will enable this in preset-env as well

@existentialism
Copy link
Member

@mathiasbynens thanks! :P

@mathiasbynens
Copy link
Contributor Author

After we land this, I have a small follow-up PR that will enable this in preset-env as well

We’ll also want to start thinking about preset-es2018, which would include this transform.

@mathiasbynens
Copy link
Contributor Author

Thanks for the help, everyone! ♥

@hzoo
Copy link
Member

hzoo commented Dec 19, 2017

preset-es2018

We are going to stop making yearly presets since we have preset-env now 😄

@hzoo hzoo merged commit 44da820 into babel:master Dec 19, 2017
@mathiasbynens
Copy link
Contributor Author

We are going to stop making yearly presets since we have preset-env now 😄

That’s even better news! 🎉

@mathiasbynens mathiasbynens deleted the dotall branch December 19, 2017 16:45
mathiasbynens added a commit to mathiasbynens/babel-plugin-transform-dotall-regex that referenced this pull request Dec 19, 2017
@mathiasbynens
Mention new location
85b78aa 
Ref. babel/babel#7059.
@existentialism existentialism mentioned this pull request Dec 19, 2017
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments

@hzoo hzoo hzoo left review comments

@xtuc xtuc xtuc approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mathiasbynens @nicolo-ribaudo @hzoo @existentialism @babel-bot @xtuc