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

fix(adapter): looks for adapter relative to git root #327

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

pmcelhaney
Copy link
Contributor

instead of npm root, which may not exist

fixes #324

instead of npm root, which may not exist

fixes commitizen#324
@AndersDJohnson
Copy link
Member

Should consider pros & cons of ShellJS + git rev-parse --show-toplevel vs. finding closest parent with .git directory e.g. via https://www.npmjs.com/package/find-parent-dir.

@LinusU
Copy link
Contributor

LinusU commented Aug 16, 2016

I don't like ShellJS at all and would love to get rid of it, but this is currently breaking for some of our users so I would like to get this landed as soon as possible. Pull request to refactor welcome :)

@LinusU LinusU merged commit 45bdd76 into commitizen:master Aug 16, 2016
@LinusU
Copy link
Contributor

LinusU commented Aug 16, 2016

Thanks for the quick response @pmcelhaney 🎉

@pmcelhaney
Copy link
Contributor Author

I prefer asking git for the root path rather than looking for the .git directory because I don't know if there's some edge case in which searching for the .git directory doesn't work. For example, is it possible to create a file called .git in a subdirectory? Is it possible to configure git so the directory is called something other ".git"?

@LinusU
Copy link
Contributor

LinusU commented Aug 16, 2016

That's true, would be nice to switch to e.g. execa so that it's not blocking but instead gives a promise. Should probably have clarified but the thing I don't like about ShellJS is the synchronous nature of it...

@pmcelhaney
Copy link
Contributor Author

I'm thinking about creating a new npm module that can find the various root / home folders across all OSes.

var rootz = require('rootz');

var git = rootz.sync.git(),
     hg = rootz.sync.hg(),
    svn = rootz.sync.svn(),
    npm = rootz.sync.npmPackage(),
   home = rootz.sync.userHome(),
    ...

// non-blocking
   rootz.git().then(function (gitRootDirectory) {
      ...
   })

What do you think? Would that be useful?

@LinusU
Copy link
Contributor

LinusU commented Aug 16, 2016

I personally like small focused packages (some reading). Maybe make it multiple packages with one package that depends on the other if someone wants that.

Then we we could to:

const gitRoot = require('rootz-git')

gitRoot().then(dir => {
  // ...
})

without having to add anything that isn't related to git to this package.

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.

catch some error when i upgrade the version to 2.8.5
3 participants