Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

Questions about contributing #43

Closed
Ledragon opened this issue Jul 17, 2016 · 10 comments
Closed

Questions about contributing #43

Ledragon opened this issue Jul 17, 2016 · 10 comments
Assignees
Labels

Comments

@Ledragon
Copy link
Contributor

Hi,
First of all, thanks for this repo, it's great to be able to find all new d3 types in a single place rather that the huge DefinitelyTyped repo.
I have a few questions:

  • I started working on the d3-request API, How do I make sure nobody else is doing it at the same time? I.e. how to avoid concurrent work?
  • How do you sync with DefinitelyTyped to make types available via Typings?
  • Do you have nomenclature/style rules that should be followed by contributors? Would it make sense to write a contributing.md file?
@tomwanzek tomwanzek self-assigned this Jul 17, 2016
@tomwanzek
Copy link
Owner

In response to the above, here are my thoughts in terms of 'Roadmap':

How do you sync with DefinitelyTyped to make types available via Typings?

While I absolutely understand where you come from with respect to the vast expanse that is DefinitelyTyped, I am working on moving the definitions/tests contributed under this repo into DefinitelyTyped. The rationale has to do with:

  • the @types organization and the new vision for definitions acquisition through npm. The @types organization is fed from a types-2.0 branch of DefintelyTyped. This branch also supports TS 2, which makes some of the 'advanced features' in the definitions viable right away.
  • it seems too cumbersome to replicate some of the testing infrastructure already in place there with Travis integration etc. and additional tooling under development from the typescript side.
  • hopefully broader community support for such a vibrant and wonderful library.

I started working on a pull-request for the modules that are finished so far move them over asap. Starting with the TS2 version into the types-2.0 branch of DefintelyTyped.

In parallel, it's all about completing the existing other modules (d3-collection, d3-force), including your valued pull request for d3-request for TS2.

As for the 'standard' DefinitelyTyped master branch for TS 1.8.x, I intended to work backwards from the TS2 version. I.e. strip out unsupported features/wrap the module definitions and submit them as a pull-request for the master branch.

As I submit the first bigger PR for DT, I will outline this approach in the already open issue for D3 v4.1.x definitions and look for feedback.

Your valued PR for d3-request would then be attributed to you as author. Note that out of gratitude for the original D3 v3 definitions, I kept the original authors listed in all the new definitions.

So in summary, I always considered this repo a transitory staging ground, while I tackled some of the integration challenges coming with the new modular structure and the new TS 2 features.

Therefore, I had not formalized a contribution guide and/or style guide specific to D3 related modules. I am totally with you on the principle, but not sure how that would best be implemented within the confines of DefinitelyTyped.

Thoughts welcome!

@Ledragon
Copy link
Contributor Author

Ledragon commented Jul 21, 2016

Thanks for the more than complete answer. Your choice of making a PR to DT makes total sense, as it would be way too hard to maintain both list of definitions in parallel. I think my question would have been better formulated as something like "How do types go to DT?"; the fact that you intend to submit a PR totally answers that.

Regarding the contributing guide, I think maybe something just related to this repo could do; once merged in DT, the story is different.

As a starting point, I suggest to include a Contributing.md file at the root of this repo, and include things such as:

  • Classes and interfaces should be in PascalCase
  • Use spaces and not tabs
  • 4 spaces for indentation
  • Should type file include JSDoc comment?
  • Should type file include reference to url of each method?
  • Include 4 lines at the top of the type file containing project and author information
  • Variables in tests must have a type to ensure correct response type

I am not myself such a writer of documentation, so this is raw idea from the top of my head.

@tomwanzek
Copy link
Owner

Thanks for your take. A mini Contributing.md should indeed do for this repo. I will look into posting one later today. The basis above is a good starting point.

The two items I held back on for now are the JSDoc comments urls to the official API documents. For some of the modules there was still quite some flux and it is a matter of triaging.

IMHO the comments can be added in when the functional definitions are already in DT, i.e. they are accessible through 'normal' definitions acquisitions methods. A very nice to have, but secondary to availability. I added some in, where something really jumped out at me when I read the API or the D3 code, that did not seem immediately obvious.

Links at the method/property level, however, always make me wonder about maintainability.

@aendra-rininsland
Copy link

aendra-rininsland commented Jul 22, 2016

Hi! I too would like to contribute!

If I might make a suggestion — perhaps provide installation instructions for people using typings, I'm still not entirely sure what the best way is. It might also be worthwhile to add a typings.json into the root of the repo so testers can just do:

$ typings install github:tomwanzek/d3-v4-definitelytyped/ --save

Awesome work on this, though — I'm so incredibly excited to be able to start using v4 in TypeScript. 😄

@tomwanzek
Copy link
Owner

I started the process of migrating the definitions that are complete with tests into the pipeline for DefinitelyTyped. More details are provided in my response to the issue definition request for D3 v4.1.x.

@Aendrew thanks for the additional offer to contribute.

I will update the README and add a 'mini' Contributing.md as per @Ledragon 's suggestion.

The focus of the update will be on explaining the current status of this repo and how to contribute until migration to DefinitelyTyped is complete.

So stay tuned.

As for typings.json. My current focus is on the migration to DT, at which point the acquisition would be one of two for normal purposes:

  1. The future of acquisition should be as simple as npm install @types/d3-selection --save as DT (currently the types-2.0 branch) feeds the @types organization supported with TypeScript 2+
  2. The regular DT master branch would contain TS 1.8.x version definitions (until phased out) which could be acquired in the traditional way including through typings.

I build the repo to develop/test the definitions in an integrated way, and frankly ran into some funky behavior when I used typings at the beginning with typings install file:... from the src directory. It started when I upgraded from 1.0.4 to 1.3.x. Could not pin it down to even explain how to replicate the issue. I then removed it from the toolchain to focus the effort. Sorry, so the typings.json in the individual definition directories are a placeholder from an earlier time, which I kept only as a fallback for reference.

@tomwanzek
Copy link
Owner

I materially updated the README with Roadmap info. The first part of the CONTRIBUTING.md is there as well. Need to put some finishing touches on it tomorrow.

Please note that I have added Travis CI to the change control process.

@tomwanzek
Copy link
Owner

Completed CONTRIBUTING.md.
@Ledragon @Aendrew Please, let me know, if this addresses what you need to know at this point. Suggestions welcome.

@Ledragon
Copy link
Contributor Author

Wow, great job! This seems to be more than the mini guide discussed, but his is just awesome.

On a side not,e to ensure style consistency, I think .editorconfig file can be used, so that the editor knows how to propagate pre-defined rules. I never did it myself, but contributed to this project that used it. It requires an extension for VS code.

@tomwanzek
Copy link
Owner

Thanks. Will consider .editorconfig, even if it just for future reference. I'll close this issue for now.

@aendra-rininsland
Copy link

@tomwanzek Nicely done! 👍 I have nothing to add, that's a very detailed README! 😄

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

No branches or pull requests

3 participants