-
Notifications
You must be signed in to change notification settings - Fork 166
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
Added option to okBuck to configure rule overrides compatible with skylark #752
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.
Lets chat about a different api for this. Not a fan of the current verbosity
* Specify override for the rule, for example: | ||
* override { | ||
* nativeRuleName="java_library" | ||
* bzlFile="tooling/defs/my_defs.bzl" |
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.
should probably be importLocation="//tooling/defs:my_defs.bzl
.
nit: we should probably validate the file structure and also that new new rule name is all snake case?
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.
- re-written documentation with full example instead of just single section
buildSrc/src/main/java/com/uber/okbuck/extension/RuleOverridesExtension.java
Outdated
Show resolved
Hide resolved
Multimap<String, String> loadStatements = | ||
createLoadStatements(visibilityExtension, hasVisibilityFile); | ||
|
||
Map<String, RuleOverridesExtension.OverrideSetting> overrides = | ||
overridesExtension.getOverrides(); | ||
for (Rule rule : rules) { |
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.
nit: can use streams
also, exported file rules append to buck files. Might have to consider creating a singleton which keeps track of all rules and writes them down in the end (similar to how dependencies are managed by DependencyManager)
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.
ack
@@ -43,6 +43,10 @@ public T ruleType(String ruleType) { | |||
return (T) this; | |||
} | |||
|
|||
public String ruleType() { | |||
return ruleType; |
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.
some one off rules/buck files don't respect this rule type.
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.
thank you for letting me know, will have to identify these and update them in the breaking change.
|
||
private void validateExtension() { | ||
Set<String> usedOverrideKeys = new HashSet<>(); | ||
for (OverrideSetting setting : overrides) { |
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.
nit: can use streams.
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 don't like throwing exceptions from the streams, stack traces tend to look strange.
}); | ||
} | ||
|
||
private void validateExtension() { |
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.
might be a good idea to have these extensions implement an interface which initializes the extension. that can then be called by the base extension to validate & setup?
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.
This is good idea, will not do it yet, since there is only 2 precedents.
task -> BuckFileGenerator.generate( | ||
bp, | ||
okbuckExt.getVisibilityExtension(), | ||
okbuckExt.getRuleOverridesExtension())); |
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 probably just pass the okbuckExt
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.
protected void override(Closure<OverrideSetting> action) { | ||
OverrideSetting setting = new OverrideSetting(); | ||
action.setDelegate(setting); | ||
action.call(setting); |
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.
seems like there is a cleaner way to do this - http://mrhaki.blogspot.com/2016/02/gradle-goodness-create-objects-with-dsl.html
Now instead of having to define java_library in DEFS one can tell okBuck to load and use my_java_library in //defs/my_defs.bzl like:
with this config specified, generates files look like this:
Description:
Related issue(s):