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

resolving module_path with __dirname #96

Merged
merged 3 commits into from
Jun 27, 2017
Merged

resolving module_path with __dirname #96

merged 3 commits into from
Jun 27, 2017

Conversation

NonPolynomial
Copy link
Contributor

What does this PR cover?

Fixing the issue #64.

  • required path - module in manager/utils.js
  • resolving the module_path with __dirname

How can this be tested?

Initialize astrum pattern llibrary in arbitary folder (e.g. astrum init ./pttrns/)

Screenshots / Screencast

astrum-fix-64-before
astrum-fix-64-after


Reviewers

Review 1

  • 👍

Review 2 (optional)

  • 👍

By adding a +1 you are confirming you have...

  • Witnessed the work behaving as expected (this could be on the author's machine or screencast).
  • Checked for coding anti-patterns.

@RyanHavoc
Copy link
Member

@NonPolynomial Thanks for this. I haven't had chance to test on a Mac yet but it looks good.

@NonPolynomial
Copy link
Contributor Author

NonPolynomial commented Feb 21, 2017

@RyanHavoc I have to thank you, I'm glad that I can contribute some to this awesome tool 👍

@mixn
Copy link

mixn commented Jun 27, 2017

Any way that this could get merged, @RyanHavoc? :)

Also, thanks for this, @NonPolynomial. I think I would have picked it up otherwise, but you were faster. :)

@giothevanni
Copy link

giothevanni commented Jun 27, 2017

Good question, I also pitched in in this fix and made a Pull Request in @NonPolynomial 's branch removing unnecessary dependencies ("global-modules") after resolving the path relatively, see https://github.com/NonPolynomial/astrum/pull/1

I'd also like to point out that this fix resolves issue #116 as well, enabling astrum users to install it locally, for example as a dev dependency, which is great for CI environments or usage with containers, not having to install the library globally in the image as well.

@giothevanni
Copy link

I tested the functionality in a Windows 7 VM and macOS Sierra successfully.

Hey @giothevanni ,

thanks for your further work on this fix!
@NonPolynomial
Copy link
Contributor Author

Hey guys,

i merged the commit from @giothevanni into my repo.

This should work now for Win 7, 8.1 and 10, and also on MacOS Sierra - thanks to @giothevanni for further fixes and testing.

FYI @RyanHavoc

@RyanHavoc RyanHavoc merged commit 51e28a4 into NoDivide:master Jun 27, 2017
@RyanHavoc
Copy link
Member

@NonPolynomial @giothevanni Thanks for the work on this guys! Released it as 1.9.5.

@NonPolynomial NonPolynomial mentioned this pull request Jun 27, 2017
@mixn
Copy link

mixn commented Jun 27, 2017

Thank you all, this was way faster than I expected. :) Setting it up locally now. Cheers! 🍻

@NonPolynomial
Copy link
Contributor Author

NonPolynomial commented Jun 27, 2017

@RyanHavoc How is the progress on Release v2.0.0? Are those changes from @giothevanni and me conflicting with the current state of release/2.0.0 ?
Can we help to fix those conflicts?

@NonPolynomial NonPolynomial deleted the fix/resolve-module_path branch June 27, 2017 17:59
@giothevanni giothevanni mentioned this pull request Aug 18, 2017
2 tasks
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.

4 participants