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

Rewrite Gephi parser into TypeScript #39

Merged
merged 3 commits into from
Jul 31, 2019
Merged

Rewrite Gephi parser into TypeScript #39

merged 3 commits into from
Jul 31, 2019

Conversation

Thomaash
Copy link
Member

Apart from obvious this also includes a bugfix: options.edges.inheritColor was originally read as options.inheritColor and therefore ignored.

Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

The TS pull-requests are really terrible to review.
You could try a git mv gephiParser.js gephiParser.ts and than do the changes? No idea if this works but atm these files are hard to review and we loose a git history of these files 😢

@Thomaash
Copy link
Member Author

I tried it. git mv works from one commit to another if there are not too many changes. Across multiple commits or with many changes (like types inserted almost everywhere) it just fails.

@mojoaxel
Copy link
Member

I tried it. git mv works from one commit to another if there are not too many changes.

But could you not just move/rename the file than commit it and than start changing using new commits? (I'f never done that. I'm just curious!)

@Thomaash
Copy link
Member Author

I did it. It works if you go commit by commit. If you want to see the entire thing it breaks anyway.

@mojoaxel
Copy link
Member

If you want to see the entire thing it breaks anyway.

But without the git mv the history of this file gets lost :-(

@Thomaash
Copy link
Member Author

But with it too. I tried to retain the history, Git just loses track once the changes are too big. And they will be too big if you squash this PR.

@mojoaxel
Copy link
Member

Thanks for clarifying!
Yes this squashing is a real destroyer of the git history...well it is how it is!
Thanks for trying!

@Thomaash
Copy link
Member Author

No problem.

@Thomaash Thomaash requested a review from mojoaxel July 30, 2019 16:58
mojoaxel
mojoaxel previously approved these changes Jul 31, 2019
Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

LGTM

@Thomaash Thomaash merged commit 35cf075 into visjs:master Jul 31, 2019
@Thomaash
Copy link
Member Author

Thomaash commented Aug 2, 2019

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Thomaash Thomaash deleted the gephi-ts branch August 2, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants