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

Add JSHint configuration file and apply many suggestions #346

Merged
merged 2 commits into from Aug 14, 2015

Conversation

Projects
None yet
3 participants
@Dandandan
Contributor

Dandandan commented Aug 13, 2015

No description provided.

@evancz

View changes

Show outdated Hide outdated src/Native/Graphics/Element.js
@@ -334,6 +334,7 @@ Elm.Native.Graphics.Element.make = function(localRuntime) {
case 'Z':
transform = 'translateX(' + ((-w/2)|0) + 'px) ';
break;

This comment has been minimized.

@evancz

evancz Aug 13, 2015

Member

These actually are supposed to fall through. Only excuse I can make is that this was written by a younger me ;P

@evancz

evancz Aug 13, 2015

Member

These actually are supposed to fall through. Only excuse I can make is that this was written by a younger me ;P

var elmValue = decoder(value[key]);
var pair = Utils.Tuple2(key, elmValue);
keyValuePairs = List.Cons(pair, keyValuePairs);
if (value.hasOwnProperty(key)) {

This comment has been minimized.

@evancz

evancz Aug 14, 2015

Member

Why does this make this code better? Seems like it'll take some extra time and either (a) not protect us from anything because of how the Elm API is designed or (b) lead to weird results where we aren't allowed to grab fields that we want. Can you explain more about this?

@evancz

evancz Aug 14, 2015

Member

Why does this make this code better? Seems like it'll take some extra time and either (a) not protect us from anything because of how the Elm API is designed or (b) lead to weird results where we aren't allowed to grab fields that we want. Can you explain more about this?

This comment has been minimized.

@Raynos

Raynos Aug 14, 2015

function Foo() {
  this.myField = null;
}
Foo.prototype.doAThing = function doAThing() {}

var f = new Foo();
for (var k in f) console.log(k); // myField, doAThing

Using a for var in loop on an object with methods is unsafe.

@Raynos

Raynos Aug 14, 2015

function Foo() {
  this.myField = null;
}
Foo.prototype.doAThing = function doAThing() {}

var f = new Foo();
for (var k in f) console.log(k); // myField, doAThing

Using a for var in loop on an object with methods is unsafe.

This comment has been minimized.

@evancz

evancz Aug 14, 2015

Member

Sure, but decoders won't let functions through, so that case never happens. So that's covered by (a)

@evancz

evancz Aug 14, 2015

Member

Sure, but decoders won't let functions through, so that case never happens. So that's covered by (a)

@evancz evancz merged commit 400aba0 into elm:master Aug 14, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 14, 2015

Member

If the hasOwnProperty stuff is better, can you do a separate PR to add it back in? I removed it because I wasn't sure how it changed the existing behavior and I have not heard of folks with problems there.

Member

evancz commented Aug 14, 2015

If the hasOwnProperty stuff is better, can you do a separate PR to add it back in? I removed it because I wasn't sure how it changed the existing behavior and I have not heard of folks with problems there.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 14, 2015

Member

Otherwise, nice work, thank you! :D

Member

evancz commented Aug 14, 2015

Otherwise, nice work, thank you! :D

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 14, 2015

Member

Also, is it possible to ask for tab indentation only? Right now it says 2, does that mean it wants spaces?

Member

evancz commented Aug 14, 2015

Also, is it possible to ask for tab indentation only? Right now it says 2, does that mean it wants spaces?

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