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

Clean up dependencies in brackets.js #10596

Merged
merged 3 commits into from May 20, 2015

Conversation

Projects
None yet
2 participants
@MarcelGerber
Contributor

MarcelGerber commented Feb 14, 2015

For case 2 of #1783.

@MarcelGerber MarcelGerber changed the title from Brackets js dependencies to Clean up dependencies in brackets.js Feb 14, 2015

CodeInspection : CodeInspection,
CommandManager : CommandManager,
Commands : Commands,
CodeHintManager : require("editor/CodeHintManager"),

This comment has been minimized.

@busykai

busykai May 13, 2015

Contributor

Why another require is preferable? Does it solve any issue at all?

@busykai

busykai May 13, 2015

Contributor

Why another require is preferable? Does it solve any issue at all?

This comment has been minimized.

@MarcelGerber

MarcelGerber May 13, 2015

Contributor

I removed the require above since we should only require module over there that we actually need in our code, for example for calling their methods.
The only occurence of CodeHintManager, though, is here in brackets.test.

See #1783 (case 2) for more details.

@MarcelGerber

MarcelGerber May 13, 2015

Contributor

I removed the require above since we should only require module over there that we actually need in our code, for example for calling their methods.
The only occurence of CodeHintManager, though, is here in brackets.test.

See #1783 (case 2) for more details.

This comment has been minimized.

@busykai

busykai May 13, 2015

Contributor

It does not quite explain all the changes. LanguageManager is loaded twice via require, for example.

@busykai

busykai May 13, 2015

Contributor

It does not quite explain all the changes. LanguageManager is loaded twice via require, for example.

This comment has been minimized.

@MarcelGerber

MarcelGerber May 13, 2015

Contributor

Oh, I see. This PR is 3 months old, so I don't exactly know the initial motivation, but I think it might be that this just looks cleaner to me, than having a mixture of requires and variable references.

@MarcelGerber

MarcelGerber May 13, 2015

Contributor

Oh, I see. This PR is 3 months old, so I don't exactly know the initial motivation, but I think it might be that this just looks cleaner to me, than having a mixture of requires and variable references.

This comment has been minimized.

@busykai

busykai May 14, 2015

Contributor

OK. Now I see. Not going to argue aesthetics. It may improve readability for this one case, but I prefer variables since they can be linted and not get back at you at runtime. Technically, there's a negligible difference and the code is executed once. So if you want to, you can merge it with master and we can merge it back in. If not, let's close it.

@busykai

busykai May 14, 2015

Contributor

OK. Now I see. Not going to argue aesthetics. It may improve readability for this one case, but I prefer variables since they can be linted and not get back at you at runtime. Technically, there's a negligible difference and the code is executed once. So if you want to, you can merge it with master and we can merge it back in. If not, let's close it.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 14, 2015

Contributor

@busykai Merged with master.

Contributor

MarcelGerber commented May 14, 2015

@busykai Merged with master.

@busykai

This comment has been minimized.

Show comment
Hide comment
@busykai

busykai May 14, 2015

Contributor

@MarcelGerber, it works OK. The only comment I have is: I'd also move the CodeMirror load/global def lines 104-114 to before line 47 (right before all the CodeMirror plugins are loaded). First of all, these lines create a global; secondly it makes sense (for readability) to load the module itself before loading its plugins. Tried the change, it causes no problems. Does it make sense to you?

Contributor

busykai commented May 14, 2015

@MarcelGerber, it works OK. The only comment I have is: I'd also move the CodeMirror load/global def lines 104-114 to before line 47 (right before all the CodeMirror plugins are loaded). First of all, these lines create a global; secondly it makes sense (for readability) to load the module itself before loading its plugins. Tried the change, it causes no problems. Does it make sense to you?

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 16, 2015

Contributor

The issue with that is, though, that these lines need DeprecationWarning, which is defined later on (and thus causes a JSLint error).

The only thing we could do is load CodeMirror before line 47 and then create the global, deprecated object afterwards, but that would then be out of context.

Contributor

MarcelGerber commented May 16, 2015

The issue with that is, though, that these lines need DeprecationWarning, which is defined later on (and thus causes a JSLint error).

The only thing we could do is load CodeMirror before line 47 and then create the global, deprecated object afterwards, but that would then be out of context.

@busykai

This comment has been minimized.

Show comment
Hide comment
@busykai

busykai May 18, 2015

Contributor

@MarcelGerber, you're right. However, I think we could well load DeprecationWarning at the very beginning (using separate var). It's a utility and does not really belong among other "functional" modules. What do you think?

Contributor

busykai commented May 18, 2015

@MarcelGerber, you're right. However, I think we could well load DeprecationWarning at the very beginning (using separate var). It's a utility and does not really belong among other "functional" modules. What do you think?

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber May 19, 2015

Contributor

I'm not too fond about that. We've pretty much always kept the requires in one block and I don't like the idea of making an exception just for this module.

Contributor

MarcelGerber commented May 19, 2015

I'm not too fond about that. We've pretty much always kept the requires in one block and I don't like the idea of making an exception just for this module.

@busykai

This comment has been minimized.

Show comment
Hide comment
@busykai

busykai May 20, 2015

Contributor

@MarcelGerber, ok! This looks good to me. It solves the purpose that you outline in the PR description. Merging.

Contributor

busykai commented May 20, 2015

@MarcelGerber, ok! This looks good to me. It solves the purpose that you outline in the PR description. Merging.

busykai added a commit that referenced this pull request May 20, 2015

@busykai busykai merged commit 19f9b43 into adobe:master May 20, 2015

1 check passed

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

@MarcelGerber MarcelGerber deleted the MarcelGerber:brackets-js-dependencies branch May 20, 2015

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