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

Hash spec (for recent IRC-spawned commit) #430

Merged
merged 2 commits into from
Nov 8, 2013
Merged

Conversation

alexch
Copy link
Contributor

@alexch alexch commented Nov 7, 2013

I figure the best way to understand a new project is to dive in and start writing tests (and fixing bugs). When I was on IRC I noticed this commit ee0caf1 come through with no tests, so...

I hope you don't mind if I submit a stream of patches while I get my bearings. (Don't expect me to be your test monkey forever, though.)

@meh
Copy link
Member

meh commented Nov 7, 2013

Could you amend the commits with a more meaningful message please?

One would have to check the specific commit to know what's going on.

@alexch
Copy link
Contributor Author

alexch commented Nov 7, 2013

How's that?

@meh
Copy link
Member

meh commented Nov 7, 2013

I'd remove the unit test for X entirely, just Add tests for Array#select passing params to the block and Add tests for Hash.new with native object containing null.

Sorry for the bikeshedding.

@alexch
Copy link
Contributor Author

alexch commented Nov 7, 2013

Well, as long as we're bikeshedding...

Don't you think it's useful to have a link back to the original commit,
which has more information and context, for a pure unit test commit?

What if I put (see xyz123) in the commit body?

@meh
Copy link
Member

meh commented Nov 7, 2013

I personally dislike binding commits to other commits, or tests to commits. The feature is there, and the unit tests are there to ensure there are no future regressions about the feature, no matter in what commit they were introduced or changed.

@alexch
Copy link
Contributor Author

alexch commented Nov 7, 2013

Okey doke.

meh added a commit that referenced this pull request Nov 8, 2013
Hash spec (for recent IRC-spawned commit)
@meh meh merged commit 1585e3d into opal:master Nov 8, 2013
@adambeynon
Copy link
Contributor

I'm not sure Hash.new() with a native object is a good idea. We are currently converting null to nil, but we should really be converting all inline objects to hashes, as well as converting each array item to a ruby object if needed.

This is already done by JSON, so this partial implementation of that on hash seems firstly a duplication, and secondly not a full implementation of something useful.

@meh
Copy link
Member

meh commented Nov 8, 2013

@adambeynon JSON doesn't just convert the object to a Hash, it also calls the json_class hooks.

But I think we should cleanup Native and move it to the stdlib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants