fixes #27 - upgraded required node version 0.10 as path.delimiter and pa... #60
Conversation
… path.sep is used now
| @@ -30,7 +30,7 @@ exports.pathMatchPatterns = function (absolutePath, pathPatterns) { | |||
| } | |||
|
|
|||
| // Check if starts with pathPattern literal | |||
| if (absolutePath.indexOf(pathPattern.replace(/\/?$/, '/')) === 0) { | |||
| if (absolutePath.indexOf(pathPattern.replace(/[\\\/]?$/, path.sep)) === 0) { | |||
There was a problem hiding this comment.
how will this behave on windows. Since on windows the path will not start with '/' but rather so 'C:'
There was a problem hiding this comment.
So on windows, path.sep will be '' and the regex there will search to see if the end of the pathPattern to see if it ends with either '/', '' or '' and then replace it with path.sep. Then there is a check to see if absolutePath begins with the modified pathPattern. So it will behave as normal from before. For example:
| absolutePath | pathPattern | result |
|---|---|---|
| C:\foo\bar\ | C:\ | true |
| C:\foo\bar\ | C:\foo\ | true |
| C:\foo\bar\ | C:\foo | true |
| C:\foo\bar\ | C:\bar | false |
| C:\foo\bar\ | C:\foo\bar | true |
There was a problem hiding this comment.
Ah my bad, seems i mis-read what you wrote. Totally thought your regex was /^[\\\/]?/. My bad. This is fine and sorry for the confusion.
|
Hi @KevinAdu, Sorry for the delay, to be honest I wrote a response not too long after I got the PR and had it in my browser. But felt like nitpicking on it a bit more and ... well my browser must've crashed and before I pressed the comment button. Losing the comment, but thinking I had completed it. Apologies for that. Apart from the question above. The PR itself looks good to go. However we will try and maintain node 0.8 compatibility for a little bit longer as we aren't in a rush to move to 0.10/0.12 just for better path management. We are thinking of creating a component version 1 branch of this project to support some of the new features component has. This will probably belong in that branch, as component 1 needs a more up to date version of nodejs. |
|
Hi @AlmirKadric, That's alright, don't worry about it. Sorry about the delayed response myself, I was travelling and then just got back a couple of days ago. Thank you for the review! I see, that makes sense. Then when that component 1 branch is created, would you like me to create a pull request for that branch? |
|
Yes please! If you don't mind waiting a little bit longer. |
|
Sure that's no problem! |
I have upgraded the required node version to 0.10 as mentioned in #27. This allows the use of path.delimiter and path.sep to make component-hint a bit more windows-friendly.
As with regards to testing, I felt that once #56 is up the possibility to test across different operating systems would be available.
If you do have any comments, please do let me know.