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

DSVRowString and DSVRowAny return undefined for non-existing rows, and DSVParsedArray return undefined for columns of empty input. #21092

Closed
wants to merge 3 commits into from

Conversation

azoson
Copy link
Contributor

@azoson azoson commented Oct 28, 2017

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <https://github.com/d3/d3-dsv/blob/master/README.md#dsv_parse>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@dt-bot
Copy link
Member

dt-bot commented Oct 28, 2017

types/d3-dsv/index.d.ts

to authors (@tomwanzek @gustavderdrache @borisyankov). Could you review this PR?
👍 or 👎?

@typescript-bot
Copy link
Contributor

@azoson Please fix the failures indicated in the Travis CI log.

@tomwanzek
Copy link
Contributor

@azoson Thanks for the contribution. As it stands the definitions for d3-request have not been officially vetted for use with strictNullChecks. (see #11365).

As you are putting your finger on an open TODO, I would suggest to use this PR as a starting point to address this is on complete sweep. I.e. we should vet the whole definition, make the appropriate changes and activate strictNullChecks in the tsconfig.json for the d3-request definitions.

This may also entail updating some of the tests.

Would you be willing to tackle this jointly as part of this PR? Would be appreciated... 🥇

@tomwanzek
Copy link
Contributor

To correct my mistake: For d3-request in my above comment, read: d3-dsv.

@azoson
Copy link
Contributor Author

azoson commented Oct 28, 2017

At first, I excuse that my poor English skill may lead misunderstanding and rude sentences.
(Even this sentence may be rude, but I cannot judge myself 😣)

I have the willingness to fix this errors in this PR.
I am very interested in your proposal.
As you said, now my changes cause some errors in the test when strictNullCheck activated.

I will update d3-dsv-test.ts.
Then check if the codes passed test.
Is there other to do?

@tomwanzek
Copy link
Contributor

No worries about your English. All good 😄 Let me have a look at d3-dsv this weekend. I haven't looked at it in a while, so I can't tell you off the top of my head, what changes may be needed to make it viable for strictNullChecks.

I'll provide you input based on what I see. So thanks for staying tuned.

@tomwanzek
Copy link
Contributor

@azoson My apologies for the brief delay. Upon closer look at the definitions for d3-dsv, I thought it would be easier to do a more complete sweep. So I am going to create a new PR as the basis for addressing both the strictNullChecks and complete JSDoc comments.

It seemed easier in this case, as the explanation of "requested/suggested" changes might have been longer and more cumbersome to follow.

I will cross-reference this PR to it, as I submit the request for review/merge. I will @-mention you alongside @gustavderdrache on the new PR for review/discussion.

Feel free to leave this PR open for now, until we align on the new PR.

@azoson
Copy link
Contributor Author

azoson commented Nov 1, 2017

Do not worry about the delay 😄
I am glad to hear that there is a better way to enable strictNullChecks.
I pushed my changes to fix the test script.
It may help your new PR.
I'm looking forward to your new PR and discussion.

@tomwanzek
Copy link
Contributor

See #21162.

@tomwanzek
Copy link
Contributor

@azoson Can this be closed now in favour of #21162? If you have any further comments/suggestions, you could add them to the new PR before it is merged. (I.p. see my point there about how to handle the columns property.) Cheers, T.

Copy link
Contributor

@tomwanzek tomwanzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azoson Let's close this PR with a big thank you as it is superseded by #21162.

@RyanCavanaugh RyanCavanaugh added the Revision needed This PR needs code changes before it can be merged. label Nov 3, 2017
@RyanCavanaugh
Copy link
Member

@azoson Please address comments from the code reviewers.

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 4, 2017

Closed as it was superseded by #21162

@rbuckton rbuckton closed this Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants