-
Notifications
You must be signed in to change notification settings - Fork 24
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
Import target determinator and test suite #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there going to be git submodules??
/bazel-* | ||
/.ijwb/ | ||
|
||
/third_party/protobuf/bazel/*/*.pb.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these files are needed to make the editor happy, you could check them in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not required, it's more of a "If you wanted to see the output" - IntelliJ works fine for me without the manual generation.
Author: Daniel Wagner-Hall <dwagnerhall@apple.com> | ||
Date: Thu Apr 21 18:21:46 2022 +0100 | ||
|
||
Only return rules (and not external ones) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an upstream PR for it yet? Ideally that PR number would go in the filename of this patch, so it's easier for us to track when it's merged and the patch can be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not - I'll happily kick off a discussion async :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, I often just create the draft PR at least as a placeholder so I don't forget to start the process of converging again with upstream
import java.util.Set; | ||
import org.junit.Ignore; | ||
|
||
public class BazelDiffIntegrationTest extends Tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a README explaining whether this is testing against all three implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 08765c6 :)
I wasn't intending for there to be - should there be? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there really not a "comment on file" feature in GitHub? I asked about git submodules because there was an empty .gitmodules
file at the root but you've removed it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've barely scanned the .go
and .java
files based on my understanding that they were already reviewed in another repo where the code originated.
@@ -0,0 +1,138 @@ | |||
# This file was manually copied from https://github.com/ewhauser/bazel-differ/blob/916e6409ae501c56ac4d6da9c0785c2ee97721b3/deps.bzl because gazelle doesn't seem to like working with dep files from remote repositories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had this issue recently as well, was it related to select
clauses in generated BUILD files?
aspect-build/bazel-examples#4 (comment)
One of us should file upstream on gazelle...
oh and we should figure out why the tests aren't triggering on this PR |
name: presubmit | ||
on: | ||
push: | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this empty block is the problem with no checks running
I think GitHub Actions can't be added in a PR from another fork, or something like that. They've been running on my fork: https://github.com/illicitonion/target-determinator/runs/6323004650?check_suite_focus=true and I expect them to start running on merge :) |
No description provided.