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 file extension replacement in node module #211

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

Conversation

parasyte
Copy link

@parasyte parasyte commented Nov 8, 2016

Rebase for #138

I discovered a bug with filename extension removal in dot.process when using additional . characters in the filename. Example directory structure:

src/video/webgl/glsl/
|-- fragment.glsl.dot
`-- vertex.glsl.dot

Processed with:

console.log(Object.keys(dot.process({
    src : "src/video/webgl/glsl/"
})));

Outputs the following keys:

["fragment", "vertex"]

The expected "glsl" extension has been removed. The patch fixes this issue by using String.prototype.replace instead of locating the first . character.

@coveralls
Copy link

coveralls commented Nov 8, 2016

Coverage Status

Coverage decreased (-9.3%) to 88.8% when pulling 69eede2 on parasyte:bug/replace-ext-rebase into e7accbf on olado:master.

@epoberezkin
Copy link
Collaborator

@parasyte thank you. let me review and I will merge. Don't worry about coverage getting down - it's actually better now as another file has the test.

@epoberezkin
Copy link
Collaborator

epoberezkin commented Nov 8, 2016

I like the change, but given that the way it works now is, arguably, "a feature" and this change may break some users' code, the change should go in v2. I think it's time to create a branch for v2 and we can release it to npm as beta. Please give me some time - I will let you know.

@epoberezkin
Copy link
Collaborator

@parasyte It would be helpful if you could submit some passing test, without code changes, as a separate PR in the meanwhile.

@epoberezkin
Copy link
Collaborator

@parasyte just created branch v2.0.0. Please submit:

  • the test that passes without the changes to the master branch
  • an additional test (that fails without changes) and the change to branch v2.0.0

Then I can merge it all and deploy 2.0.0 as beta version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants