cfjedimaster / brackets-jshint

Adds JSHint support to Brackets
MIT License
131 stars 41 forks source link

Now checks the package.json for JSHint options. #34

Closed qubyte closed 9 years ago

qubyte commented 10 years ago

Addresses #26.

busykai commented 10 years ago

I am not exactly sure why all the code has been moved to callbacks. Any particular reason for that?

qubyte commented 10 years ago

Chaining promises looks messy to my eyes. Given that the promise that was there was being used completely internally anyway, I didn't see a reason to keep it given that what I was doing needed some refactoring.

busykai commented 10 years ago

JQuery deferred is the model used by Brackets and soon it will be used by this extension as well (it will be returning a promise to Brackets). This change is a no-go as far as I'm concerned. There's no valid reason to switch to callbacks (order/mess perceptions left aside). If the issue is pressing for you, you could use your version of the extension and then switch to the new one node-jshint lookup algorithm is properly supported. Would that work for you?

qubyte commented 10 years ago

I can update the pull request to yield a promise if needs be, but I'd like @cfjedimaster's blessing first. I'm from a node background and new to Brackets. Could you clarify your involvement in brackets-jshint for me please @busykai?

cfjedimaster commented 10 years ago

To be honest, this feels a bit above me. ;) In general, I agree that promises make more sense. I don't use promises very often as I'm still wrapping my head around them, but I do think they are better.

As for busykai's involvement, he has been helpful in a few of my extensions. :)

qubyte commented 10 years ago

Ok. The difficulty here is in unifying multiple promises or async calls into a single promise. I'll take a look after work, but if there's a code sample you can point me to that wraps parallel or series asynchronous calls up in a promise that would be helpful for me.

Off the top of my head I'm only coming up with a promise wrapper around the async code I've already got in this PR. If that's the way to do it, then I don't see much point to adding that code until it's needed.

qubyte commented 10 years ago

(putting this here for me later) I think what I need is in here: https://github.com/adobe/brackets/blob/master/src/utils/Async.js

Lots of hoops for a bit of async...

busykai commented 10 years ago

@qubyte you're overcomplicating. async is not needed. no parallel execution needed. if i get it right, package.json should take precedence over .jshintrc, it's sequential. if you're up to it, could you see if #35 works for you (i was at this meeting and had some time to hack).

to answer your question, however, if you need to wait on more than one promises to proceed, there's $.when() which would allow you to do that.

qubyte commented 10 years ago

I am already doing the file reads in series. Did you read this pull request thoroughly?. Async is needed of course, since promises are just wrappers for async. Your code has duplicated all the same async bits (file.read) as mine, with lots of logic in anonymous functions nested in the file.read calls. Essentially it's just the async-wrapped-in-a-promise I mention above. Neither PR will have much affect on changes that need to be made for later versions of Brackets.

@cfjedimaster I'll get what I need either way. If you would like me to update this pull request I can have it ready this evening (UTC). If not, please close it.

@busykai One-upping me was not necessary. I state above that I plan to update this pull request if required. You have made this an unpleasant experience.

busykai commented 10 years ago

@qubyte it was not my intention to one-up you. i am sorry you're taking it this way and i apologize. i mean it. i just wanted to show how it could have been done with much less changes (twice less, to be precise). on the other hand i do care how this project is coded and i'd like it to follow certain principles which your PR does not seem to respect. i still believe, btw, that it should wait until async providers are supported (the implementation then will look quite different).

Neither PR will have much affect on changes that need to be made for later versions of Brackets.

my only comment wrt this is that I'm not refactoring every project i see to be a binary code just because i have background in programming using punchcards (as much as i would like to :). there are many changes in your pull request that have nothing to do with the problem it is trying to solve.

I am already doing the file reads in series.

i saw that, yes. why you need to use Brackets' Async then? or wait on multiple promises.

Did you read this pull request thoroughly?

this, as well as couple of other phrases, is not helping discussion. i did read your request thoroughly enough.

cfjedimaster commented 10 years ago

My gut says if a future (relatively soon) Brackets core update will make part of this obsolete, then I'd rather wait. To be fair, most of my JSHint config is in the file itself so this is a feature that wouldn't impact me much.

qubyte commented 10 years ago

@busykai I just can't see how adding another pull request was intended to be helpful. How was I supposed to take it? Again, I have said that I will update mine to put a promise back in if it is deemed necessary. I did not purposefully refactor out a (1!) promise. This is just what happened whilst trying to keep my code dry. The change to tryLoadConfig was only because the promise had fallen out. I'm not trying to disrespect the project in any way. Please tell me what changes I have made that are not to do with the problem in hand. The whitespace changes are my editor, which is set to trim them.

I was considering Brackets async because it may be worth trying to load both sources in parallel and then pick based on what comes back. I was at work though so I didn't have time to look properly or think it through completely. I was initially wondering if there was some async.series -> promise helper that would make this a lot neater.

What is the harm in adding functionality here if it's going to look different when async providers are added in anyway? My code will likely get wiped out at that point. This is currently a blocker to me and others in my organisation who I would like to make the jump to brackets.

I have had enough of being talked down to, so I won't add anything else to this comment thread. @cfjedimaster, please close this pull request if it will not be needed. I will update it if you want, but I'll assume not.