-
Notifications
You must be signed in to change notification settings - Fork 28
Fix scala_deps_direct and scala_deps_used #258
Fix scala_deps_direct and scala_deps_used #258
Conversation
@@ -67,91 +67,6 @@ configure_bootstrap_scala(<a href="#configure_bootstrap_scala-name">name</a>, <a | |||
</table> | |||
|
|||
|
|||
<a name="#configure_zinc_scala"></a> |
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.
The downside is the stardoc has less info.
@@ -535,3 +535,16 @@ configure_zinc_scala = rule( | |||
}, | |||
implementation = _configure_zinc_scala_implementation, | |||
) | |||
|
|||
def configure_zinc_scala(**kwargs): |
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.
what's the reasoning behind wrapping the original configue_zinc_scala? couldn't we just use these selects on the original attributes instead? That way we don't lose the documentation
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.
As far as I can tell, select
only applies in BUILD
or macro. select
needs to be resolved before passing to a rule
.
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.
Ah it seems you're right..
(bazelbuild/bazel#287 (comment))
That's unfortunate though cause it means we'll lose the signature docs.. an option is to explicitly propagate each argument instead of using **kwargs
, but we'd still lose the argument types/descriptions so I'm not sure that'd be much better
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.
Yeah, those explicitly listed attributes will only be shown optional
.
James said it used to work, but I really doubt 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.
It might be good to add a separate doc page for configure_zinc_scala
and to link to it from the README. You could simply copy the old md and put it in its own file (and perhaps include a comment on how you can use deps_used_off
and deps_direct_off
to disable things.
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.
The document has been added.
<tr id="configure_zinc_scala-deps_direct"> | ||
<td><code>deps_direct</code></td> | ||
<td> | ||
String; optional |
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.
Can you add the options here too?
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.
Fixed.
<tr id="configure_zinc_scala-deps_used"> | ||
<td><code>deps_used</code></td> | ||
<td> | ||
String; optional |
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.
and here
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.
Fixed.
Looks great! I'll approve once builds pass |
It has passed. |
Fix: #249