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

Geo stream #9

Merged
merged 7 commits into from
May 23, 2016
Merged

Geo stream #9

merged 7 commits into from
May 23, 2016

Conversation

chrisuehlinger
Copy link
Contributor

I just got legal approval for the Contributor Agreement, so here's my first contribution!

In reference to Issue #7, this PR contains the implementation of d3.geoStream, ported over from the v3.5.17 release of d3/d3.Tests have been ported to tape and relevant documentation has been brought over as well.

One thing to note: The first test in the stream suite was passing in v3.5.17 because the assert.fail() was throwing an error which was then caught by the try block. I rewrote that test in a way that I think preserves the intent, but it could probably use a second look.

@mbostock
Copy link
Member

Well done! Two trivial things I noticed immediately:

  • Please use two-space indentation.
  • The d3_geo_ prefix is no longer needed now that we are using ES2015 modules. That was necessary with SMASH because SMASH simply concatenated everything into a shared namespace. It looks like a find-and-remove will work here. (Occasionally I rename things for clarity, but these names seem okay.)

@mbostock mbostock self-assigned this May 23, 2016
@chrisuehlinger
Copy link
Contributor Author

No problem!

I'll change my editor's settings to reflect the 2-space indentation.

@mbostock
Copy link
Member

More trivial style nits:

  • Don’t include the file extension (“.js”) in import statements.
  • Don’t put a space between function and the open paren (().
  • Don’t put a space after the opening brace ({) in an object literal.
  • Don’t put a space before the closing brace (}) in an object literal.
  • Do use a trailing newline at the end of every file.
  • Use test.equal instead of test.isEqual.
  • You don’t need to call test.pass.

Other things:

  • The test “stream does not allow null input” should be called “stream ignores null input”, and catching the exception should be unnecessary since an uncaught exception will cause the test to fail naturally.

I’ll make these changes locally and then merge to save you the trouble.

@mbostock
Copy link
Member

Actually, I’m just going to delete that test, since there’s already a test for null input geometries. That’s the bogus test you uncovered, anyway. 😁

@mbostock mbostock merged commit efc7b7d into d3:master May 23, 2016
@mbostock
Copy link
Member

Thanks for the contribution!

@mbostock mbostock mentioned this pull request May 23, 2016
@chrisuehlinger chrisuehlinger deleted the geo-stream branch June 7, 2016 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants