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

d3-geo Update to version 1.5 #14756

Closed
tomwanzek opened this issue Feb 21, 2017 · 10 comments
Closed

d3-geo Update to version 1.5 #14756

tomwanzek opened this issue Feb 21, 2017 · 10 comments

Comments

@tomwanzek
Copy link
Contributor

d3-geo has seen a minor version update to 1.5 adding a new method.

The definitions should be updated to reflect path.measure(...) and any other relevant changes since the last version against which d3-geo was validated.

The minor version of d3 definitions should also be bumped to 4.6. Could be done in the same PR.

@Ledragon
cc @gustavderdrrache

@Ledragon
Copy link
Contributor

@tomwanzek wanted to ping you about this. If modified in d3-geo, do definitions bubble up to the global d3 definitions? Also, I think d3-hierarchy had some minor changes recently (not sure if the API changed though)...

@tomwanzek
Copy link
Contributor Author

@Ledragon Until recently, we pinned the definition's minor version for the standard D3 bundle in d3 to the corresponding minor versions of the d3 modules included in the bundle. This was done with a package.json file stub, which contained only the relevant dependencies.

This would imply that after updating the d3-geo definitions to 1.5 and publishing it, we would bump the minor version of the d3 definitions to 4.6. We would also have updated the package.json dependency on d3-geo to 1.5.

So this would be a two step process in consecutive PRs. At least, that is how I have done it in the past.

Now, recently, to my surprise, I found that the d3 package.json pinning the minor versions down in a meaningful way, was removed without my awareness. IMHO it should be re-added to keep the minor versions in the bundle aligned with the minor version of the bundle definition. I had raised this question with the folks at MSFT who made the change, but did not receive a response.

I guess it is time to follow up once more on that. So, to remove any confusion I inadvertently created, when I said in my initial comment under this issue, that d3-geo and d3 minor version could be bumped in the same PR, this is only permissible without the package.json version pinning.

As for the question of "global" d3, if you are referring to the d3 global exported in the d3 definitions, it will automatically pull the latest (pinned) version of d3-geo in as a re-export.

The standalone definitions for d3-geo themselves, do not export to a d3 global due to TypeScript duplicate identifier issues.

As for d3-hierarchy, I already made and published the changes up to the latest minor version.

@Ledragon
Copy link
Contributor

Ledragon commented Feb 23, 2017

Okay, thanks for the reply. I see indeed that the package.json was removed; won't it make any problem when installing the global d3 definitions?

As for the 'global' question, yes, I was referring to the re-export

@Ledragon
Copy link
Contributor

As of today, 1.6 is out, with the addition of geoContains. As the previous PR was not merged yet, should I try to get it in at the same time?

@tomwanzek
Copy link
Contributor Author

I saw it as well, I am tempted to say, let's process this one first so we maintain a version correspondence without gap. Then we do 1.6 as a follow-up PR. Let me go through this one to o.k. it. (Sorry, I had some unexpected "fun" with tensorflow 1.0 on the weekend.) I am prioritizing #14756 for today to push it through.

@Ledragon
Copy link
Contributor

Ok, that works for me.
Whenever you have time is fine, I don't think there is a rush on this

@tomwanzek
Copy link
Contributor Author

Alright, finally turned around on my part (see comments in PR).

I will tackle the update to d3-geo 1.6 afterwards and let you review. So you can take revenge for my slight delay in getting back to this 😉

@Ledragon
Copy link
Contributor

Ledragon commented Mar 5, 2017

I updated the PR, thanks for your comments. Hopes it gets merged soon, so that 1.6 can be tackled...

minestarks added a commit that referenced this issue Mar 7, 2017
@tomwanzek
Copy link
Contributor Author

Thx to @Ledragon ! I am closing this issue in favour of its successor #15054

@Ledragon
Copy link
Contributor

Ledragon commented Mar 8, 2017

Thx!

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

No branches or pull requests

2 participants