Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fixed: LanguageManager shouldn't throw exceptions #3034

Merged
merged 6 commits into from
Mar 19, 2013

Conversation

DennisKehrig
Copy link
Contributor

Fix for #2967

@ghost ghost assigned jasonsanjose Mar 8, 2013
@jasonsanjose
Copy link
Member

Reviewing


var finish = function () {
if (!CodeMirror.modes[mode]) {
result.reject("CodeMirror mode \"" + mode + "\" is not loaded");
return;
return result.promise();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to return promise to the finish callback right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, absolutely right!

@jasonsanjose
Copy link
Member

Initial review complete

@DennisKehrig
Copy link
Contributor Author

@jasonsanjose Thanks for the review! I fixed the problems you found.

@DennisKehrig
Copy link
Contributor Author

@jasonsanjose JSHint doesn't like my code, but JSLint does. Should we configure JSHint with "laxbreak: true" or should I update my code? Not sure how, though, I wouldn't want all this to be in one line:

    if (!language._setId(id)
            || !language._setName(name)
            || (blockComment && !language.setBlockCommentSyntax(blockComment[0], blockComment[1]))
            || (lineComment && !language.setLineCommentSyntax(lineComment))
            ) {

@TomMalbran
Copy link
Contributor

How about this?

if (!language._setId(id) || !language._setName(name) ||
        (blockComment && !language.setBlockCommentSyntax(blockComment[0], blockComment[1])) ||
        (lineComment && !language.setLineCommentSyntax(lineComment))) {

Looks and works in jshint.org and JSLint.

@DennisKehrig
Copy link
Contributor Author

@TomMalbran Looks good to me, thanks!

@jasonsanjose
Copy link
Member

Changes look good. Just need to fix the merge conflict.

* text=auto
src/nls/zh-cn/strings.js binary
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3164

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you omit this change here, will the merge conflicts go away? Raymond is reviewing your other pull.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you mean? Without this change, git thinks I modified the Chinese translation and checks that in - and I don't want to mess anything up this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same problem when I merged last night after the translation was merged, but merging again this morning (by removing the translation file) the problem fixed by itself, the translation was properly readded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, pulled a fresh master and with that I can now create new branches and check them out without git complaining.

@DennisKehrig
Copy link
Contributor Author

Supposedly the Chinese translation was changed by this... I'll try rebasing.

@DennisKehrig
Copy link
Contributor Author

Looking better.

@DennisKehrig
Copy link
Contributor Author

Purified version - no more messing with .gitattributes. Thanks @TomMalbran!

@jasonsanjose
Copy link
Member

Confirmed unit tests pass after trying a test merge with master. Merging. Thanks for your patience @DennisKehrig.

jasonsanjose added a commit that referenced this pull request Mar 19, 2013
Fixed: LanguageManager shouldn't throw exceptions
@jasonsanjose jasonsanjose merged commit 34f973f into adobe:master Mar 19, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants