Dependencies #3
Dependencies #3
Conversation
OliverFriedrich
force-pushed
the
dependencies
branch
from
06043c7
to
c975532
Compare
|
Would be great, if you could update the npm package. Than i just need to update my dependencies instead of replacing the package ... |
There was a problem hiding this comment.
Ok to integrate your changes. Especially, the CommonJS/AMD integration is something I completely ditched when writing this mode (and others) so your contribution on this matter is welcome.
However I have highlighted a series of details that must be fixed beforehand.
When you submit a pull request, the changes you made to the code will of course be reviewed and discussed. But the way you push these changes will equally be reviewed and discussed: number of commits + what each commit contains: is each change atomic? Are commit messages relevant and well written?
This makes sense considering Git enables you to completely rewrite history (git rebase is your friend) whenever needed. In the end, it is customary to submit an artificial history with perfect/elegant commits.
src/pcre.js
Outdated
| /* global CodeMirror, define */ | ||
|
|
||
| (function (mod) { | ||
| if (typeof exports === "object" && typeof module === "object") // CommonJS |
There was a problem hiding this comment.
pcre.js is indented using tabs therefore I would appreciate if you could use tabs for these few extra lines.
There was a problem hiding this comment.
no problem, have copied these lines from some existing mode script, so the spaces where already used there. But that can be changed
There was a problem hiding this comment.
Indention of the file corrected. This leads to a complete replace of the original code as each line gets an additional tab in front of it...
src/pcre.js
Outdated
|
|
||
| (function (mod) { | ||
| if (typeof exports === "object" && typeof module === "object") // CommonJS | ||
| mod(require("codemirror/lib/codemirror")); |
There was a problem hiding this comment.
Should it not be ../../lib/codemirror instead of codemirror/lib/codemirror ? The idea here is to remain consistent with upstream CodeMirror modes, e.g. https://github.com/codemirror/codemirror5/blob/fc0440bc7563aead662492b0f1b0512637545442/mode/asn.1/asn.1.js#L6
There was a problem hiding this comment.
This would be true if this mode script would be part of the package "codemirror", but it isn't. So it is not possible/useful to use relative paths to access the elements in codemirror package.
So it is accessed by package name and path. CommonJS handles the loading ...
Ok -- once your pull request is accepted, I intend to:
|
OliverFriedrich
force-pushed
the
dependencies
branch
from
ccce0ee
to
3cedda6
Compare
Add dependency injection around existing code to make in usable in Laravel/Vue.js projects