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

Rename CodeMirror2 folder to CodeMirror #11150

Merged
merged 1 commit into from May 23, 2015

Conversation

Projects
None yet
5 participants
@MarcelGerber
Contributor

MarcelGerber commented May 21, 2015

Now that we use CodeMirror 5, it's a little misleading our submodule is still called CodeMirror2. Thus, in this PR, I've renamed it to CodeMirror.

There are some things to keep in mind, though:

  • While most extensions will still work through our require mapping, other ways to access CodeMirror2 (FileSystem, node) won't work. Look out for those.
  • We probably cannot utilize deprecation warnings.
  • See if any extension accesses CodeMirror2 through anything else than a require (FileSystem, node, ...)
  • See if we can fire deprecation warnings
  • See if it makes sense to rename our adobe/CodeMirror2 repo as well
@rroshan1

This comment has been minimized.

Show comment
Hide comment
@rroshan1

rroshan1 May 23, 2015

Contributor

I too had been contemplating updating the CodeMirror submodule name and its side-effects. Merging the changes.
Have raised an issue to track the action items mentioned in the description post this merge.

Contributor

rroshan1 commented May 23, 2015

I too had been contemplating updating the CodeMirror submodule name and its side-effects. Merging the changes.
Have raised an issue to track the action items mentioned in the description post this merge.

rroshan1 added a commit that referenced this pull request May 23, 2015

@rroshan1 rroshan1 merged commit 3d68a76 into master May 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcelGerber MarcelGerber deleted the marcel/codemirror2-to-codemirror branch May 23, 2015

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose May 23, 2015

Contributor

We could have delayed this merge till the next release and give a heads-up in this release about the impending change; This could be a breaking change with some extensions. Also the three issues mentioned by marcel had to be verified before this change was merged.
I think we should revert this change for this release to allow extension devs enough time to verify their extensions or alternatively check the impact of this change before merge.
Created a revert just in case we decide to revert this for the release.
@prafulVaishnav @nethip

Contributor

abose commented May 23, 2015

We could have delayed this merge till the next release and give a heads-up in this release about the impending change; This could be a breaking change with some extensions. Also the three issues mentioned by marcel had to be verified before this change was merged.
I think we should revert this change for this release to allow extension devs enough time to verify their extensions or alternatively check the impact of this change before merge.
Created a revert just in case we decide to revert this for the release.
@prafulVaishnav @nethip

@abose abose restored the marcel/codemirror2-to-codemirror branch May 23, 2015

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 23, 2015

Contributor

There's one extension that uses CodeMirror2 for literal access: Brackets-Themes by @MiguelCastillo - all the others only use brackets.getModule.

Knowing this, I think we don't need to revert the changes. Miguel is probably pretty fast in updating his extension so it checks whether CodeMirror2 or CodeMirror should be used.

Point 2: We don't really need them (it's a "nice to have" and nothing else) as we can simply leave the requirejs rerouting in.

Point 3: That's just our infrastructure and it's very easy and non-breaking to do.

Contributor

MarcelGerber commented May 23, 2015

There's one extension that uses CodeMirror2 for literal access: Brackets-Themes by @MiguelCastillo - all the others only use brackets.getModule.

Knowing this, I think we don't need to revert the changes. Miguel is probably pretty fast in updating his extension so it checks whether CodeMirror2 or CodeMirror should be used.

Point 2: We don't really need them (it's a "nice to have" and nothing else) as we can simply leave the requirejs rerouting in.

Point 3: That's just our infrastructure and it's very easy and non-breaking to do.

@MiguelCastillo

This comment has been minimized.

Show comment
Hide comment
@MiguelCastillo

MiguelCastillo May 23, 2015

Contributor

@MarcelGerber Thanks for roping me in. I will change update as soon as this is merged in. :) Thanks for the heads up.

Contributor

MiguelCastillo commented May 23, 2015

@MarcelGerber Thanks for roping me in. I will change update as soon as this is merged in. :) Thanks for the heads up.

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose May 23, 2015

Contributor

Thanks Marcel, i'll run a search for the usage of codeMirror2 on the source codes in the extension registry just in case and update. If the extension usage is proper, there would be nothing to worry about this going in the release 🐹

Contributor

abose commented May 23, 2015

Thanks Marcel, i'll run a search for the usage of codeMirror2 on the source codes in the extension registry just in case and update. If the extension usage is proper, there would be nothing to worry about this going in the release 🐹

@MiguelCastillo

This comment has been minimized.

Show comment
Hide comment
@MiguelCastillo

MiguelCastillo May 23, 2015

Contributor

Alright, I just created a PR that's ready to be merged in when this is merged. MiguelCastillo/Brackets-Themes#115

Contributor

MiguelCastillo commented May 23, 2015

Alright, I just created a PR that's ready to be merged in when this is merged. MiguelCastillo/Brackets-Themes#115

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 23, 2015

Contributor

@MiguelCastillo This PR is already merged, but it's not yet part of a Brackets Release.
One other thing you could possibly do is check whether CodeMirror2 still exists and use the appropiate directory based on the result. Just a suggestion, though.

@abose That's actually what I already did, but nothing to say against a second pair of eyes :)

Contributor

MarcelGerber commented May 23, 2015

@MiguelCastillo This PR is already merged, but it's not yet part of a Brackets Release.
One other thing you could possibly do is check whether CodeMirror2 still exists and use the appropiate directory based on the result. Just a suggestion, though.

@abose That's actually what I already did, but nothing to say against a second pair of eyes :)

@MiguelCastillo

This comment has been minimized.

Show comment
Hide comment
@MiguelCastillo

MiguelCastillo May 23, 2015

Contributor

@MarcelGerber oh man, I must be going crazy. I could not find the merged comment! I see it now that I went through the thread in more details... I wondering why I couldn't see the merge button :)

Contributor

MiguelCastillo commented May 23, 2015

@MarcelGerber oh man, I must be going crazy. I could not find the merged comment! I see it now that I went through the thread in more details... I wondering why I couldn't see the merge button :)

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 24, 2015

Contributor

@MiguelCastillo Notice though that your PR will only work in Brackets, so better set your package.json's minimum Brackets version to 1.4, too.

That's why I suggested conditionally detecting what path to use above: It will still work for both versions post- and pre-1.4.
In order to make that work, don't yet change the brackets.getModule paths - CodeMirror2 will still work fine for now.

Contributor

MarcelGerber commented May 24, 2015

@MiguelCastillo Notice though that your PR will only work in Brackets, so better set your package.json's minimum Brackets version to 1.4, too.

That's why I suggested conditionally detecting what path to use above: It will still work for both versions post- and pre-1.4.
In order to make that work, don't yet change the brackets.getModule paths - CodeMirror2 will still work fine for now.

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip May 24, 2015

Contributor

@MarcelGerber Thanks for fixing this. This was a very good observation.

And thanks to @MiguelCastillo and @abose for collaborating on making sure this won't effect the other modules.

Since these are all architectural changes I feel we should use our slack channel to bring these up so that everyone can put in their opinions before merging these kind of changes to master.

Contributor

nethip commented May 24, 2015

@MarcelGerber Thanks for fixing this. This was a very good observation.

And thanks to @MiguelCastillo and @abose for collaborating on making sure this won't effect the other modules.

Since these are all architectural changes I feel we should use our slack channel to bring these up so that everyone can put in their opinions before merging these kind of changes to master.

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose May 25, 2015

Contributor

@marcel, I ran a search on the extension registry for CodeMirro2 and it seems that all of them except Miguel's extension is using Bracket.getModule("path?Codemirror2/file") format. So this change would be safe for this release as you said.
I will go ahead and delete the reverting PR.

Contributor

abose commented May 25, 2015

@marcel, I ran a search on the extension registry for CodeMirro2 and it seems that all of them except Miguel's extension is using Bracket.getModule("path?Codemirror2/file") format. So this change would be safe for this release as you said.
I will go ahead and delete the reverting PR.

@MarcelGerber MarcelGerber deleted the marcel/codemirror2-to-codemirror branch May 25, 2015

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