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

feat(transformers): add tests #4153

Merged
merged 18 commits into from Nov 21, 2019
Merged

feat(transformers): add tests #4153

merged 18 commits into from Nov 21, 2019

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Oct 1, 2019

Edited the existing codemod to add tests, and ran it on the codebase. This is as a reference for other transformers we can add in the future.

I could not find how to merge two calls unfortunately :(

Note: this PR used to include a CLI, which is now removed, since we disagree whether it should be part of this project.

This CLI is mostly copied from react-codemod, which is BSD licensed, so I need to look at where to put the license text.

Since the CLI has dependencies, which I just added in teh dependencies field, it might make sense to have a separate repo. I like the fact that you can run it from the dependencies directly though, so to consider.

While testing this out I found a few more changes, which were applied in the next commit.
scripts/transforms/README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/transforms/index.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

added some things i thought of

scripts/transforms/index.js Outdated Show resolved Hide resolved
scripts/transforms/index.js Outdated Show resolved Hide resolved
scripts/transforms/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

looks good to me

@Haroenv Haroenv removed the request for review from samouss October 7, 2019 14:19
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

I'm wondering if this codemod provides value if it's not transforming to this:

// Input
search.addWidget(instantsearch.widgets.hits({}));
search.addWidget(instantsearch.widgets.hits({}));

// Output
search.addWidgets([
  instantsearch.widgets.hits({}),
  instantsearch.widgets.hits({})
]);

As a user, how would you benefit from using it if you need to edit the files on your own afterwards?

jest.config.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/transforms/README.md Outdated Show resolved Hide resolved
Haroenv and others added 2 commits October 21, 2019 10:12
Co-Authored-By: François Chalifour <francoischalifour@users.noreply.github.com>
@francoischalifour
Copy link
Member

francoischalifour commented Oct 21, 2019

@Haroenv As expressed in #4153 (review), I wouldn't be comfortable introducing a new usage of the package for exposing a first codemod that does not fully address the problem. Right now, it makes the code work with v4 but leaves it in an "unstable" state where you still call multiple times addWidgets(). In my mind, we should solve the problem fully and not provide a non-complete solution.

@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 21, 2019

Merging function calls isn't something that seems to be in any way trivial with codemods, since you can't really go to the "next call" as far as I can tell

@Haroenv Haroenv changed the title feat(transformers): add CLI feat(transformers): add tests Nov 13, 2019
@Haroenv Haroenv changed the base branch from next to develop November 13, 2019 09:34
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

If I understand correctly the intent of this PR has changed since its creation. It now only deals with testing the transformer and does not introduce the CLI.

@Haroenv
Copy link
Contributor Author

Haroenv commented Nov 21, 2019

correct, CLI was removed in a later commit since we disagreed :)

@Haroenv Haroenv merged commit 0787769 into develop Nov 21, 2019
@Haroenv Haroenv deleted the feat/transformer-cli branch November 21, 2019 15:39
tkrugg pushed a commit that referenced this pull request Nov 26, 2019
* feat(transformers): add CLI

This CLI is mostly copied from react-codemod, which is BSD licensed, so I need to look at where to put the license text.

Since the CLI has dependencies, which I just added in teh dependencies field, it might make sense to have a separate repo. I like the fact that you can run it from the dependencies directly though, so to consider.

While testing this out I found a few more changes, which were applied in the next commit.

* chore: additional changes found by fix in script

* Update addWidget-to-addWidgets.test.js

* Update scripts/transforms/README.md

* chore: add license

* chore: move dependency

* chore: add to package.json

* chore: replace chalk with inline

* fix typo

* Update scripts/transforms/README.md

Co-Authored-By: François Chalifour <francoischalifour@users.noreply.github.com>

* move file structure less complex

* Update scripts/transforms/README.md

* remove CLI

* chore: ordering

* docs: wording of call
Haroenv added a commit that referenced this pull request Nov 28, 2019
## [4.0.1](v4.0.0...v4.0.1) (2019-11-28)

### Bug Fixes

* widget name in documentation link for index ([#4172](#4172)) ([fe7e588](fe7e588))
* **helper:** rely on stable version of algoliasearch-helper ([#4200](#4200)) ([ff11731](ff11731))
* **infiniteHits:** correct widget options types ([#4222](#4222)) ([bb1b327](bb1b327))
* **queryHook:** restore behaviour of queryHook ([#4202](#4202)) ([7bf96cb](7bf96cb)), closes [/github.com/algolia/instantsearch.js/commit/c073a9acb51fff3c15278fcd563e47fec55c8365#diff-530222e0c4597f2110dc6ba173a306b0L98](https://github.com//github.com/algolia/instantsearch.js/commit/c073a9acb51fff3c15278fcd563e47fec55c8365/issues/diff-530222e0c4597f2110dc6ba173a306b0L98)

### Features

* **transformers:** add tests ([#4153](#4153)) ([5a28415](5a28415))
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.

None yet

4 participants