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

adopt pull-requests fom vis and timeline-plus #36

Closed
16 tasks
mojoaxel opened this issue Aug 2, 2019 · 7 comments
Closed
16 tasks

adopt pull-requests fom vis and timeline-plus #36

mojoaxel opened this issue Aug 2, 2019 · 7 comments
Labels
enhancement New feature or request

Comments

@mojoaxel mojoaxel added the enhancement New feature or request label Aug 2, 2019
@yotamberk
Copy link
Member

yotamberk commented Aug 2, 2019

#5 is ready to go.

@yotamberk yotamberk changed the title adopt pull-requests fom vis and visjs-network adopt pull-requests fom vis and timeline-plus Aug 2, 2019
@mojoaxel
Copy link
Member Author

mojoaxel commented Aug 3, 2019

As far as I see it we now can go two directions now:

Just merge #5 and hope that everything works and things generally improve.

  • 👍 It is the fastest way for improving the library and release a new version.
  • 👍 All changes have been testes in timeline-plus by @yotamberk and others.
  • ❕ Needs a Major version increase because we can not be sure what actually has changed. A detailed update of HISTORY and release notes would be not possible.
  • ❕ The git history of the unique PR is not visible in the new branch. Everything would be something like "merged timline-plus features (Merge timeline plus #5)".
  • ❕ There is no attribution to the original contributors of these improvements and fixes. All these dozens of PRs and changes would be attributed to @yotamberk.

We merge all vis and timeline-plus pull requests one by one

The idea is to merge (cherry-pick) every PR from vis and timeline-plus one by one. While merging we could have a look at the [merge-timeline-plus branch] to see how things should look in the end.

  • 👍 We can test and release every PR and make sure everything works fine after each merge.
  • 👍 We would have a nice git history and could "blame" every change to a specific PR. This allows us to create a detailed release HISTORY.
  • 👍 We could have attribution to the original contributors of each PR.
  • ❕ A lot of work, that would propably need a few weeks to complete (60+ PRs, each 10-20 minutes).
  • ❕ Some PRs would be hard to merge and would require a lot of looking into the [merge-timeline-plus branch].

Hey everybody! What do you think?

@mojoaxel
Copy link
Member Author

mojoaxel commented Aug 3, 2019

I personally like the idea more to merge every PR seperatly and test and release often. We did the same thing in vis-network and it worked out fine. We would very much rely on the [merge-timeline-plus branch] that @yotamberk created to see how things should look in the end. This work was NOT in vane!
The main reasons I prefer this method is the attribution and the git-history. Also I would feel more stable to release one feature by one.

I also could live with just merging #5 and make a v6 release. This would basically mean a break with the old git-history and an new start. It depends if you folks want to see this library progress more fast or more stable! I don't want to decide this!

@jczacharia
Copy link
Member

jczacharia commented Aug 3, 2019

I agree with @mojoaxel that merging in increments will be more stable but I also think that if major features are missing in vis-timeline that are only available in timeline-plus then developers will be more inclined to just stick with using timeline-plus.

Also, I ran a smoke-test of vis-timeline with the merge-timeline-plus branch and I am getting the exporting errors e.g. export 'Timeline' was not found in 'vis-timeline'.

I haven't been able to test @yotamberk merges but b/c of exporting issues but I see that this repo does not include Typescript typings index.d.ts which was very helpful to me in timeline-plus.

@murraybauer
Copy link

As a user I'd be happy with a major v6 release that merged timeline-plus without full history.

As a maintainer even if you do 5-6 cherry picked PR's an hour when there's 60+ that still a lot of time to ask that could be spent elsewhere. The layers of sediment for viewing would still exist in an archived repo when needed.

@yotamberk
Copy link
Member

Hey guys,
Although in theory the second option is a safer and has a better tracking of the history etc. It is A-LOT of work that I simply don't want to do... I've done a lot of work for timeline-plus and to resubmit all the changes done there one by one will be a pain-staking job that is simply not on my agenda or priority list. This will take me many weeks to finish.
Maybe if I get paid for my time working on it I might consider doing it, if it's really that important for/to the community, but it will still require a few weeks of work.

I know it's not ideal, but I recommend the first option. It will allow me and the community to pursue the work on this module without opting out to use timeline-plus.
Once we accept this PR, I will declare deprecation to timeline-plus and dedicate all my time on this module. We have still lots of work to get this module where it should be. I don't want to waste more time...

@mojoaxel
Copy link
Member Author

mojoaxel commented Aug 4, 2019

@yotamberk I'm going you fully support your decision as the main maintainer of this library!
Lets try to merge #5 in the near future and progress from there! 👍

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

No branches or pull requests

4 participants