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(process): add cli parameters for importing directly to sphere #40

Merged
merged 20 commits into from
Sep 7, 2016

Conversation

junajan
Copy link
Contributor

@junajan junajan commented Aug 24, 2016

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 66b0dee on TOOM-2130-product-type-import into cd9050d on master.

@junajan
Copy link
Contributor Author

junajan commented Aug 24, 2016

Integration tests don't work because of missing sphere.io credentials in travis env variables.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 253d716 on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 253d716 on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 9f20130 on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling ed574c7 on TOOM-2130-product-type-import into cd9050d on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling ed574c7 on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling fde65e2 on TOOM-2130-product-type-import into cd9050d on master.

config =
sphereClientConfig: options

Promise.resolve config
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for Promise.resolve. You can just return config here since you are already in the then callback.
Also in line 49

@coveralls
Copy link

coveralls commented Aug 26, 2016

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 6c34f70 on TOOM-2130-product-type-import into cd9050d on master.

@PhilippSpo
Copy link
Contributor

@junajan can you fix the breaking test also? Would be awesome 😊

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 1571e16 on TOOM-2130-product-type-import into cd9050d on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.821% when pulling 1571e16 on TOOM-2130-product-type-import into cd9050d on master.

@@ -1,28 +1,32 @@
language: node_js
node_js:
- '0.10'
- '0.12'
- '0.10'
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this. And we should test on 4 as well

sphereClient.productTypes.byId(productType.id)
.delete(productType.version)
.then ->
done()
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: you can keep the chain as a flat structure

importer.init(argv)
.then -> sphereClient.productTypes.fetch()
.then (res) ->
  console.log "Deleting old product types", res.body.results.length
  Promise.map res.body.results, (productType) ->
    sphereClient.productTypes.byId(productType.id)
    .delete(productType.version)
.then done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated here 48a32c2

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 86.624% when pulling 8a42cf5 on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 86.624% when pulling 8a42cf5 on TOOM-2130-product-type-import into cd9050d on master.

importer = null
sphereClient = null

before ->
Copy link
Member

Choose a reason for hiding this comment

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

You're doing async stuff here, you have to use the done callback.

Copy link
Contributor Author

@junajan junajan Sep 6, 2016

Choose a reason for hiding this comment

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

I switched tests to mocha which is working with Promises too - so it uses promise instead of done callback

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 86.164% when pulling 5fcb461 on TOOM-2130-product-type-import into cd9050d on master.

silent: !! argv.logSilent
streams: [
{ level: 'error', stream: process.stderr }
{ level: argv.logLevel || 'info', path: "#{argv.logDir || '.'}/#{package_json.name}.log" }
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to use path.join here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added here 09fdc1b

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 86.164% when pulling e866a9d on TOOM-2130-product-type-import into cd9050d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.08%) to 86.42% when pulling 09fdc1b on TOOM-2130-product-type-import into cd9050d on master.

@PhilippSpo
Copy link
Contributor

OK LGTM now 👍

@junajan junajan merged commit 3df9c68 into master Sep 7, 2016
@junajan junajan deleted the TOOM-2130-product-type-import branch February 12, 2018 13:17
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

5 participants