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

Remove is-ci in favor of ci-info #579

Merged
merged 1 commit into from
Oct 12, 2019
Merged

Conversation

xenyal
Copy link
Contributor

@xenyal xenyal commented Sep 9, 2019

Instead of having is-ci call ci-info's ci.isCI, we might as well call ci.isCI within the installation directly. This allows us to shorten the dependency branch by 1 step which is a trivial amount of gain, but it nonetheless reduces the length of the package-lock

@xenyal xenyal force-pushed the remove_is_ci branch 2 times, most recently from 719ad39 to 44c778e Compare September 9, 2019 14:25
@typicode
Copy link
Owner

Thanks!

@@ -13,6 +13,9 @@ debug(`INIT_CWD: ${process.env.INIT_CWD}`)
// huskyDir is ONLY used in dev, don't use this arguments
const [, , action, huskyDir = path.join(__dirname, '../..')] = process.argv

// Determine whether environment is CI
const { isCI } = ci.isCI
Copy link
Owner

Choose a reason for hiding this comment

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

Except if I'm missing something, shouldn't it be const { isCI } = ci based on https://github.com/watson/ci-info#usage?

Copy link
Contributor Author

@xenyal xenyal Oct 7, 2019

Choose a reason for hiding this comment

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

Based on the library's usage documentation, ci is the encapsulating namespace of the library, and isCI is the method that actually shows whether or not the call was invoked in a CI environment or not (ref: https://github.com/watson/ci-info#ciisci). Not sure if that answers your question, I'll keep an 👀out for further comments! Also, sorry for the late response!

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { isCI } = ci.isCI
const { isCI } = ci

Looks correct to me based on the linked documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@typicode @fearphage Updated - thank you both for the replies! 😄

@@ -1,5 +1,5 @@
import chalk from 'chalk'
import isCI from 'is-ci'
import ci from 'ci-info'
Copy link
Contributor

@fearphage fearphage Oct 9, 2019

Choose a reason for hiding this comment

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

More of a preference really, but just pull this out during import? ¯\_(ツ)_/¯ (Sorry I missed this earlier).

Suggested change
import ci from 'ci-info'
import { isCI } from 'ci-info'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it, updated!

Copy link
Contributor

@fearphage fearphage left a comment

Choose a reason for hiding this comment

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

Optional suggestion, but good to go otherwise.

@typicode
Copy link
Owner

Thanks @fearphage, @xenyal

@typicode typicode merged commit 89b0ffc into typicode:master Oct 12, 2019
@xenyal xenyal deleted the remove_is_ci branch October 21, 2019 15:29
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