Skip to content
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

Make code pass lint #55

Closed
wants to merge 10 commits into from
Closed

Make code pass lint #55

wants to merge 10 commits into from

Conversation

@Eeems
Copy link

Eeems commented May 13, 2014

I've touched up all the JavaScript files so they would pass a lint, and so they wouldn't throw error if you ran them in strict mode. I've also done a tiny bit of optimizing of some of the small functions that could be cut down to a single return statement instead of a small if block. I may go through and do cpu/memory optimizations later.

@ivantodorovich

This comment has been minimized.

Copy link

ivantodorovich commented May 13, 2014

Waiting to see @feross reaction.. 👯 lol

@transitive-bullshit

This comment has been minimized.

Copy link
Member

transitive-bullshit commented May 13, 2014

Note that most of the code in this repo (lib/*) is stale and has been deleted in my latest pull request because it now all resides in the isolated and more up-to-date bittorrent-client repository.

What linter are you using and with what options? Certainly need to wait on @feross, but in general, I'd recommend adding a jshintrc as good practice so everyone's on the same page about style expectations going forwards.

@Eeems

This comment has been minimized.

Copy link
Author

Eeems commented May 14, 2014

I'd have to pull up the one I'm using in sublime to see. A lot of the linting I'm doing is in my head before it actually finishes parsing and let's me know what lines are bad.

I'll check out your pull and re-lint it as well. A JavaScript spec would be nice since I see numerous styles throughout the code (which is why I ignored the horrendous overuse of 'var self = this'.
Making sure your code would run in strict mode is best, even if you aren't using strict mode actively. Its just good coding practice.

Eeems added 3 commits May 14, 2014
Conflicts:
	bin/cmd.js
	index.js
	lib/storage.js
	lib/torrent-manager.js
	lib/torrent.js
Conflicts:
	bin/cmd.js
	index.js
	lib/torrent-manager.js
	lib/torrent.js
@Eeems

This comment has been minimized.

Copy link
Author

Eeems commented May 14, 2014

Not everything passes lint anymore since I'm pulling the changes you made over.

@gauravsaini

This comment has been minimized.

Copy link

gauravsaini commented May 14, 2014

How can I check whether it is running from terminal ?
npm start and then ?

@transitive-bullshit

This comment has been minimized.

Copy link
Member

transitive-bullshit commented May 14, 2014

npm start --help

or if you want, just pass it a torrent id such as this leaves of grass infohash to start downloading a torrent
npm start 'd2474e86c95b19b8bcfdb92bc12c9d44667cfa36'

@transitive-bullshit

This comment has been minimized.

Copy link
Member

transitive-bullshit commented May 14, 2014

If it works, you should get a nice auto-updating terminal UI until the torrent finishes downloading.

@feross

This comment has been minimized.

Copy link
Member

feross commented May 14, 2014

After merging @fisch0920's changes it looks like this PR doesn't do much. So I'll close it for now.

Note that the current code style is intentional. No semicolons, function blocks whenever possible, var self = this preferred instead bind.

When sending PRs, make sure to match the style of the existing code and you'll be fine. 😺

@feross feross closed this May 14, 2014
@feross

This comment has been minimized.

Copy link
Member

feross commented May 14, 2014

I could make a simple lint module (webtorrent-lint?) that you could run in any of of the bittorrent-* repos, if you you think that would help newbies.

@Eeems

This comment has been minimized.

Copy link
Author

Eeems commented May 14, 2014

The problem with no semi-colons is that this is javascript, not python. There are many cases where forgetting a semicolon will break your code. http://www.codecademy.com/blog/78-your-guide-to-semicolons-in-javascript

Self is fine is some cases, but when you define it and do nothing with it you are just wasting CPU cycles and memory. In truth using a variable to reference this will be a problem for performance/memory usage and IMO should only be used if you need to preserve scope somehow (referencing this inside an anonymous function properly etc). When we are working on something that we care about having good performance on (Like this project) we should never sacrifice performance for a coding style that is just a matter of preference.

@Eeems

This comment has been minimized.

Copy link
Author

Eeems commented May 14, 2014

Updated to work with the last merge.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.