deprecated jvm_flags not reflected in scala_library_suite or scala_test_suite
#621
Comments
|
Isn’t there a kwargs there? Don’t remember and AFK
…On Thu, 27 Sep 2018 at 18:18 Andreas Herrmann ***@***.***> wrote:
The README marks the jvm_flags attribute to scala_library and scala_test
as deprecated. But, the corresponding scala_library_suite and
scala_test_suite macros don't reflect this deprecation, as they only
accept the jvm_flags attribute.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#621>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF1CjA0b0pLd-pDfmMK-qgbrgm7Xiks5ufOxhgaJpZM4W85Tw>
.
|
|
@ittaiz No, e.g. passing However, passing that attribute to |
|
We don’t use these utilities and I guess @johnynek on the other hand does
use the flag so it fell between the cracks.
@johnynek any objections to propagating a kwargs dictionary?
We often do this in internal macros
…On Fri, 28 Sep 2018 at 12:16 Andreas Herrmann ***@***.***> wrote:
@ittaiz <https://github.com/ittaiz> No, e.g. passing scalac_jvm_flags to
scala_library_suite causes the following error:
unexpected keyword 'scalac_jvm_flags' in call to scala_library_suite
However, passing that attribute to scala_library does not cause an error.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#621 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFxCFIgTdewnLX62FF76WBkT6UM0bks5ufekAgaJpZM4W85Tw>
.
|
|
The |
|
This sounds like a really small PR (+test).
Any chance you can contribute it?
There is a small chance @johnynek will have a good reason why not to do
this but the change is small so I’d take my chances if I were you.
In any case if he won’t respond in the next couple of days we can proceed.
…On Fri, 28 Sep 2018 at 17:58 Andreas Herrmann ***@***.***> wrote:
The plugins attribute is not passed on either. A kwargs dict seems like a
good way to cover all these.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#621 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF0c4vDe1a-DFPntMKz1_cYXP67aXks5ufjktgaJpZM4W85Tw>
.
|
|
I think it was just an oversight that we didn’t plumb them through. A PR with a test would be great. |
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 1, 2018
From `scala_library_suite` to `scala_library`, and from `scala_test_suite` to `scala_test`. closes bazelbuild#621
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 1, 2018
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 17, 2018
From `scala_library_suite` to `scala_library`, and from `scala_test_suite` to `scala_test`. closes bazelbuild#621
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 17, 2018
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 30, 2018
From `scala_library_suite` to `scala_library`, and from `scala_test_suite` to `scala_test`. closes bazelbuild#621
aherrmann
added a commit
to aherrmann/rules_scala
that referenced
this issue
Oct 30, 2018
ittaiz
pushed a commit
that referenced
this issue
Oct 30, 2018
* Forward **kwargs From `scala_library_suite` to `scala_library`, and from `scala_test_suite` to `scala_test`. closes #621 * Add regression test for #621 * Disable linting See #622 (comment) * Remove manually forwarded arguments These are now forwarded as part of the kwargs argument. Additionally, the data argument is now forwarded to scala_test_suite, instead of being quietly ignored as it was before. This addresses the following comment #627 (comment) An additional advantage is that attribute defaults don't have to be kept in sync between the *_suite and non-suite rules/macros. closes #627 * Regression test for data attr in scala_test_suite Modifies the TestSuitePassesKwArgs test-case to test that the data attribute is passed through by scala_test_suite. This is a stronger test-case since it doesn't just test that attributes like data, jvm_flags, and scalac_jvm_flags are accepted by scala_test_suite. But, it also tests that data and jvm_flags are actually passed through to scala_test. The test-case tries to read from the file defined in these attributes. * Rename test targets Addressing the following comment #622 (review)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The README marks the
jvm_flagsattribute toscala_libraryandscala_testas deprecated. But, the correspondingscala_library_suiteandscala_test_suitemacros don't reflect this deprecation, as they only accept thejvm_flagsattribute.The text was updated successfully, but these errors were encountered: