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

Importer: Clean up section-import #2951

Merged
merged 1 commit into from
Feb 3, 2016
Merged

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 1, 2016

  • Trim constants via ES6 module exports/imports
  • Merge render logic into render()
  • General cleanup

Testing This shouldn't change any functionality, so if you attempt an import and something borks out, it's broken. Otherwise it should be coding changes only.

The PR depends on #2949 and will shrink considerably once that's merged in.

cc: @apeatling @rodrigoi @omarjackman @dllh @designsimply

@dmsnell dmsnell added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Import labels Feb 1, 2016
@dmsnell dmsnell added this to the Import: First Release milestone Feb 1, 2016
@dmsnell
Copy link
Member Author

dmsnell commented Feb 1, 2016

cc: @rralian

@dmsnell dmsnell force-pushed the update/importer/clean-up-section branch 2 times, most recently from 4450564 to 57a33b5 Compare February 1, 2016 21:53
 - Trim constants via ES6 module exports/imports
 - Merge render logic into `render()`
 - General cleanup

fixup! Importer: Clean up section-import

Remove Makefile because no tests exist
@dmsnell dmsnell force-pushed the update/importer/clean-up-section branch from 57a33b5 to 813159d Compare February 2, 2016 00:50
@dllh
Copy link
Member

dllh commented Feb 2, 2016

I gave this a quick test drive and discovered no regressions. Would prefer to leave code review to others. Maybe @omarjackman or @jordwest could take a peek?

test:
@NODE_ENV=test NODE_PATH=$(NODE_PATH) $(MOCHA) --compilers $(COMPILERS) --reporter $(REPORTER) --ui $(UI)

.PHONY: test
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this file intentionally deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was left over from another PR where it should have been deleted. there are no tests left so the Makefile should go too

@jordwest
Copy link
Contributor

jordwest commented Feb 3, 2016

Gave it a test locally, also couldn't see any regressions. Code changes LGTM 🚢

@jordwest jordwest added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 3, 2016
dmsnell added a commit that referenced this pull request Feb 3, 2016
@dmsnell dmsnell merged commit c9b7502 into master Feb 3, 2016
@dmsnell dmsnell deleted the update/importer/clean-up-section branch February 3, 2016 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants