[CLOSED] Move PathUtils to Brackets core code?

[core-ai-bot]

Issue by ficristo
Monday Sep 21, 2015 at 18:42 GMT
Originally opened as adobe/brackets#11726

---

I wondered what was PathUtils and found that was coming from
https://github.com/jblas/path-utils
It does not seem used outside Brackets, at least there isn't a package.json or bower.json.
Seems written by and Adobe guy, so, can be relicensed (I think this is a must but not sure) and imported?
My main motivation is very simple: I do not like global variables.

[core-ai-bot]

Comment by petetnt
Monday Sep 21, 2015 at 20:29 GMT

---

👍 for this.

Semi-offtopic, but I think that that regexp is amazing / like the comment says, scary:

/^(((([^:\/#\?]+:)?(?:(\/\/)((?:(([^:@\/#\?]+)(?:\:([^:@\/#\?]+))?)@)?(([^:\/#\?\]\[]+|\[[^\/\]@#?]+\])(?:\:([0-9]+))?))?)?)?((\/?(?:[^\/\?#]+\/+)*)([^\?#\.]*(?:\.[^\?#\.]+)*(\.[^\?#\.]+)|[^\?#]*)))?(\?[^#]+)?)(#.*)?/,

[core-ai-bot]

Comment by abose
Tuesday Sep 22, 2015 at 06:17 GMT

---

👍
Pathutils seems to be a submodule. As we are already shipping with the lib, I don't think licensing would be an issue if we are to change the folder where the library is placed in code- as long as the copyright notices are left untampered.

[core-ai-bot]

Comment by le717
Tuesday Sep 22, 2015 at 14:42 GMT

---

We would just need to be sure to continue exposing the global variable with a deprecation warning (like we do with the global CodeMirror) in order to prevent sudden extension breaking.

I am trying to think where PathUtils would be relocated to. I'm thinking either src/file or src/utils. The former already has FIleUtils there, but once the old file system is (eventually) removed, it will be the only module in that folder, but the latter might work better (since it is a utils module).

[core-ai-bot]

Comment by zaggino
Wednesday Sep 23, 2015 at 21:32 GMT

---

Guys, this brings out a question for me ... why do we use git submodules instead of bower/npm modules? For codemirror, mustache, ...

[core-ai-bot]

Comment by busykai
Wednesday Sep 23, 2015 at 23:49 GMT

---

Submodules allow you keep forks of the code and make modifications to them without publishing anywhere. (Take CodeMirror or jslint as an example.) I actually quite disagree that #11734 had to be merged. There's no single technical reason for that: path-utils being submodule has nothing to do with PathUtils being global? If you don't like globals, then the only thing needed to be done was to configure path for requirejs.

Also, commits in #11734 do not specify where it's been taken from, at which version/sha it the snapshot has been taken.

[core-ai-bot]

Comment by zaggino
Thursday Sep 24, 2015 at 00:06 GMT

---

That's the point@busykai -> do we really need or want to keep forks and make modifications to thirdparty libraries? Take the https://github.com/adobe/CodeMirror2 example, what code do we have in there, which is not present in the upstream https://github.com/codemirror/CodeMirror ? Github says This branch is 53 commits behind codemirror:master so why not just use https://www.npmjs.com/package/codemirror if we don't keep any code specific to Brackets? Also, in extreme cases, you can use npm to point to a github repository ...

[core-ai-bot]

Comment by ficristo
Thursday Sep 24, 2015 at 05:32 GMT

---

Personally, apart the globals, I do not like submodules too.
And I think using bower (or npm) is better...
Maybe there was a good reason in the past, but now, are they still needed?

[core-ai-bot]

Comment by petetnt
Thursday Sep 24, 2015 at 05:35 GMT

---

@busykai the original PathUtils explicitly exposes itself to the window so modifications would have been necessary anyway.

I agree that the commits could have had the versions they were taken originally from, but then again the repo has been stale for 4.5 years so I am not sure if it's that relevant (unless the original PathUtils for whatever reason gets completely overhauled or gets an meaningful update at this point which I doubt).

[core-ai-bot]

Comment by ficristo
Thursday Sep 24, 2015 at 05:42 GMT

---

@busykai@petetnt you are right about the commit, I didn't thought about it...

[core-ai-bot]

Comment by zaggino
Thursday Sep 24, 2015 at 06:50 GMT

---

I really doubt@jblas is planning to make changes to his 4 years old code anytime soon ...

[core-ai-bot]

Comment by busykai
Thursday Sep 24, 2015 at 15:11 GMT

---

@zaggino, if you don't have a fork you never have a space to maneuver. If I remember correctly, we needed a modified version of CodeMirror on numerous occasions. jslint is another example. If you're using an npm version (i.e. a publicly distributed version) -- you're stuck. By stuck I mean if you fix it in the dependency upstream, you have to wait for 1) your contribution to be reviewed and accepted; 2) new package to be promoted and published. Logistically, it does not make any sense to wait for any of that. I don't know why would you assume it would never happen, it happened many times now.

I really doubt@jblas is planning to make changes to his 4 years old code anytime soon ...

why not submit a PR to path-utils and clarify the doubt? before taking any action. instead, a 1K PR to brackets is accepted within 2 hours after it is submitted.

@petetnt

the original PathUtils explicitly exposes itself to the window so modifications would have been necessary anyway.

so I could not even figure it was modified. why not contribute the modification upstream? that the open source for you: you contribute to somebody else's code so others could take advantage. if the repository is stale and inactive then you could copy it. in this case i haven't event figured it was modified (because you have nothing to compare it to, no history). the code actually says:

// This file was originally taken from:
// https://github.com/jblas/path-utils/blob/master/path-utils.js

So is it modified or not? Go figure...

@ficristo

Personally, apart the globals, I do not like submodules too.

i really don't think we should be using "I like" as an argument. :) one person's "like" vs another's "don't like" are counterproductive discussions. what make sense and what does not should the ruling force. submodules are enormously useful for our projects, like it or not.

overall, this was worth a discussion before the PR was merged. it should not be done in haste. we should at the very least move the code to thirdparty because it is a third-party code, we have a special place for it. it even has a different license (3-clause BSD).

[core-ai-bot]

Comment by petetnt
Thursday Sep 24, 2015 at 15:48 GMT

---

@busykai Well yeah, the modification comment was about needing to do more than the only thing needed to be done was to configure path for requirejs. whether it was in the original repo or in Brackets. Of course there is a great chance here of getting stuck in the upstream exactly like you described in your reply to@zaggino, but I agree that contacting@jblas should have been the first step to take (like it was implied in@ficristos first post), which I guess means that the merging might have been too hasty.

I also agree that pulling the code and then modifying the code in separate commits would have made sense to have a cleaner slate for library.

Basically the only part I disagree with is that "I do not like" is a perfectly valid argument against globals because globals are bad, period 🐹

[core-ai-bot]

Comment by abose
Thursday Sep 24, 2015 at 17:55 GMT

---

Cant we git submodule to thirdparty folder and then just use the library normally instead of using global? like using require(thirdparty/pathutils)?

[core-ai-bot]

Comment by petetnt
Thursday Sep 24, 2015 at 18:20 GMT

---

@abose like I mentioned above, the file exposes itself as window.PathUtils so it needs to be changed somewhere (like@ficristo did when it ported the file to Brackets repo). Otherwise it's possible as long as someone does the module conversion and it gets merged (I can do the PR tomorrow myself if someone doesn't do it first).

[core-ai-bot]

Comment by jblas
Thursday Sep 24, 2015 at 18:33 GMT

---

Hey guys, Adobe guy here. :-) The path-utils was written by me on my own time (not Adobe's) hence the license not mentioning Adobe. I also contributed parts of it to jquery-mobile which I was working on at the time. In any case, I tend to write my utils so they are agnostic to the environment they are used in. What exactly are you guys after here? A way to just pull it in via git submodule?

I'm hesitant to add a dependency on requirejs in path-utils.js for the reason I mentioned above, but if it helps you guys out I could checkin a version of path-utils-[something].js that uses requirejs.

[core-ai-bot]

Comment by abose
Thursday Sep 24, 2015 at 18:45 GMT

---

Or could we contribute to a new branch in the repo, and submodule to that branch from brackets so that the main repo remains tidy?

[core-ai-bot]

Comment by jblas
Thursday Sep 24, 2015 at 18:51 GMT

---

@abose sure that's fine.

[core-ai-bot]

Comment by ficristo
Thursday Sep 24, 2015 at 18:53 GMT

---

Hi@jblas. I thought the library was used only for Brackets so I wanted to have in the core instead of a module that seemed unused.
But I think having a bower.json and a package.json would be helpful.
At least I could experiment with Bower in Bracket for my own amusement.

@busykai You are right about personal tastes.
I opened this issue exactly for the discussion, but I don't know how you all manage reviews on Brackets, so I can't comment on the haste or not. And for the license, I am not a lawyer so...
I have also written Maybe there was a good reason in the past, but now, are they still needed? which I can't really understand from your answer.When I read that you do not want to wait PR get merged upstream (JSLint, CodeMirror, ...), I think why are you in haste (sorry couldn't resist) for these PR when in Brackets there are still very old PR?

[core-ai-bot]

Comment by busykai
Thursday Sep 24, 2015 at 20:00 GMT

---

Or could we contribute to a new branch in the repo, and submodule to that branch from brackets so that the main repo remains tidy?

given@jblas agrees, i believe it's what needs to be done.

[core-ai-bot]

Comment by jblas
Thursday Sep 24, 2015 at 20:17 GMT

---

@busykai@abose I was also going to mention that the other option is to just fork the path-utils and just have brackets pull in your fork (with requirejs added) as a submodule? Either way I'm fine, though the latter means you can make changes to your hearts delight and not have to rely on me for anything. :-)

[core-ai-bot]

Comment by ficristo
Friday Sep 25, 2015 at 17:20 GMT

---

@jblas Will you accept PR to add environment detection? And maybe add something like browserify to automate the tests?

[core-ai-bot]

Comment by petetnt
Monday Dec 21, 2015 at 18:32 GMT

---

@ficristo As of jblas/path-utils#1 PathUtils is now a CommonJS/AMD compatible module if you want to redo this 👍

[core-ai-bot]

Comment by ficristo
Tuesday Dec 22, 2015 at 08:20 GMT

---

@petetnt good job!
I prefer to wait adobe/brackets#12006 and use package.json directly if possible.