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

Implements custom commands #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implements custom commands #45

wants to merge 3 commits into from

Conversation

searls
Copy link
Contributor

@searls searls commented Dec 3, 2016

Users can now specify custom commands. This had been documented previously as using a : separator between module name and command in the string, but it was not implemented. Additionally, that strategy wouldn't have worked for git-repo-URLs (as a protocol or port would contain a : before the name was complete). So instead, I created a simple object format (with properties name, repoUrl, and command) that can be specified alongside strings.

I did my best not to editorialize on style or do any other refactors in the process, so even though this is a significant change, I tried to make it as non-disruptive to you and your style as I could.

A couple notes:

  • The module was previously splitting on : for non-URL module names, but since it had no useful effect previously, I doubt anyone will be broken by the fact I removed it in this commit
  • npm install https://github.com/testdouble/testdouble.js works, so perhaps the custom work with ggit isn't necessary? I only glanced at it.
  • A default timeout of 10 seconds seems much too short. I wasn't able to get much at all to finish in 10 seconds and it seemed like an unnecessary constraint placed by the module on users when a longer install doesn't suggest a failure
  • It's pretty clear that dont-break doesn't delete test folders, at least not when things fail. (oops, this is a dupe of Delete install folder on done #33)

@bahmutov
Copy link
Owner

bahmutov commented Dec 6, 2016

I agree with the format - that is something I wanted to do for a while.
I am also thinking the timeout could be a property per dependent project (or using default timeout, which I just increased to 3 minutes). Maybe rebase it against the latest code that has your 0.12 support branch merged?

@searls
Copy link
Contributor Author

searls commented Dec 7, 2016

Because of all the other changes being merged and the amount of impact in the second of my commits, I just had to do a merge instead of rebasing. In any case it should be clean to merge at this point

@bahmutov
Copy link
Owner

@searls is this still relevant? We have just merged a few pull requests that do custom test command

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

2 participants