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

ApiDiff implementation not relying on diffutils. #346

Merged
merged 2 commits into from Jul 7, 2017

Conversation

Projects
None yet
4 participants
@Krever
Contributor

Krever commented Jul 7, 2017

This solves #285.

I have copied the Dotty implementation directly and put it as private object inside existing APIDiff , so the namespace/api is not polluted. Let me now if you find some other approach more convenient(e.g. putting code directly or removing unused parts).

@jvican

LGTM, and thank you again! This is a really useful contribution (/cc @smarter that may be interested in the fix).

I'm really excited about this change, it does really improve the way people will figure out what changes triggered Zinc compiles.

@Krever

This comment has been minimized.

Show comment
Hide comment
@Krever

Krever Jul 7, 2017

Contributor

The output that was produced by scripted source-dependencies/as-seen-from-a:

snapshot1

Contributor

Krever commented Jul 7, 2017

The output that was produced by scripted source-dependencies/as-seen-from-a:

snapshot1

@Duhemm

Thanks again @Krever!

The changes LGTM, I would just like to get confirmation from someone who's more knowledgeable than me about licensing to confirm that this PR is okay as it is.

My understanding is that we should put the actual copyright notice from dotty before the code that we borrowed?

So I'm requesting changes until someone can confirm that it's okay...

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Jul 7, 2017

Member

I think crediting is enough. The compiler reporter we have is also copy pasted from Scalac, and has the same license as Dotty.

I think this is completely safe to merge because Zinc used the Scala license (BSD 3-way license). That's why we don't have to add a new license to the repo.

Member

jvican commented Jul 7, 2017

I think crediting is enough. The compiler reporter we have is also copy pasted from Scalac, and has the same license as Dotty.

I think this is completely safe to merge because Zinc used the Scala license (BSD 3-way license). That's why we don't have to add a new license to the repo.

@Krever Krever dismissed stale reviews from Duhemm and jvican via b3078da Jul 7, 2017

@Duhemm

Just a really minor comment. Thanks for the follow up on the PR!

@Duhemm

Duhemm approved these changes Jul 7, 2017

@jvican

jvican approved these changes Jul 7, 2017

And... Merging! 🎉

@jvican

jvican approved these changes Jul 7, 2017

@jvican jvican merged commit 529ca2d into sbt:1.0 Jul 7, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@jvican jvican added the spree label Jul 7, 2017

cunei pushed a commit to cunei/zinc that referenced this pull request Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment