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

Make namespace resolution safer #32

Merged
merged 1 commit into from
Feb 13, 2015
Merged

Make namespace resolution safer #32

merged 1 commit into from
Feb 13, 2015

Conversation

whitlockjc
Copy link
Contributor

* Instead of blindly turning a file extension into a RegExp, we now
  escape the input using `escape-string-regexp`.  Since it was already
  in use, it was a non-issue.

* This is a follow up to #31.  #31 fixed the immediate problem but left
  yeoman-environment in the same state it was before.  This should make
  namespace resolution safer.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.77% when pulling 5f66726 on whitlockjc:safer-namespace-identification into 765664a on yeoman:master.

@whitlockjc
Copy link
Contributor Author

The good ole Coveralls.io 422...

@SBoudrias
Copy link
Member

Actually, I though about this before merging, but as . will match any character and we know we replace the last ($) occurrence of the file extension, then we know for sure we'll always match the . character.

Just saying this won't change the behavior, but it is safer for eventual future changes.

SBoudrias added a commit that referenced this pull request Feb 13, 2015
@SBoudrias SBoudrias merged commit 3a03cc9 into yeoman:master Feb 13, 2015
@whitlockjc
Copy link
Contributor Author

I agree completely, just happy to help.

@SBoudrias
Copy link
Member

@whitlockjc If you feel like helping some more, we've had an issue open for some time about fixing our unit tests (the global NODE_PATH that bit you yesterday) #4 - or really any issues opened with the actionnable tag.

@whitlockjc
Copy link
Contributor Author

I'll take a look.

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

3 participants