Closed GoogleCodeExporter closed 9 years ago
This is a good idea. The main question is do we want this to be included in
2.8.2 or not. That release is already long overdue so we don't want to wait too
much for new features. We won't be able to get the release out this week,
though, so there is some time still.
Some comments about the current code and this feature in general.
1) Tests and docs needed.
2) It would probably be a good idea to wait until we get cleanup related to
issue 1500 ready before the final implementation. There are going to be changes
to same code that may make merging complicated and at least mess the history.
3) With Jython you should always import Java classes/interfaces like `from
foo.bar import SomeStuff` instead of just using `import foo.bar`. The former is
the style we use in Robot code in general and, more importantly, the latter
doesn't always work when the code is packaged to JAR. I don't remember the
details about this latter issue but better safe than sorry.
4) Static Java keywords cannot support named argument syntax even though they
could support kwargs. Thus your change not to create ArgumentResolver with
`resolve_named=False` is not a good idea at least alone. With that change you
probably could call Java keywords like `arg1=foo` or `arg2=bar`
(JavaArgumentParser uses generic names `arg1`, `arg2`, etc.) but that would
likely fail later. I'm not entirely sure what would be the best way to avoid
this. Perhaps we should discuss that in IRC.
5) If kwargs can be supported by having Map as the last argument, it would be
logical if varargs would work with List. If I remember correctly, currently
only arrays are supported. Jython should automatically coerce both to array and
List, so implementing this should only require recognizing List as varargs like
arrays. This should probably be taken care by a separate issue.
3)
Original comment by pekka.klarck
on 20 Nov 2013 at 8:59
2) Succesfully merged with issue 1500 cleanup.
3) Done.
Complete diff:
https://bitbucket.org/userzimmermann/robotframework/compare/javakwargs..master#d
iff
Original comment by zimmermann.code@gmail.com
on 25 Nov 2013 at 11:52
5) Done. Opened issue 1586. And merged into this issue's code. Complete diff
URL above stays valid.
Original comment by zimmermann.code@gmail.com
on 25 Nov 2013 at 6:34
Original comment by pekka.klarck
on 28 Nov 2013 at 8:48
Original comment by pekka.klarck
on 28 Nov 2013 at 8:48
Merged with r6c826f58e0b2
==> Includes cleaned up issue 1586 code.
Original comment by zimmermann.code@gmail.com
on 28 Nov 2013 at 4:14
On a quick look the code looks fine. A problem with pulling it is that the
branch now contains so much other stuff, such as several merges with the main
repository, that it is very hard to track what's coming in.
Would you be interested in creating a new repository or branch with just the
changes that are needed on top of the current code? Another possibility is that
we pull your changes, merge them, and then take a diff of the changes and merge
it as a patch to the mainline.
I can clearly see how Git with its features for cleaning up history would be
useful with contributions like this. We have made a decision to move Robot from
Google Code to GitHub or BitBucket sooner than later, and will also move it to
Git at the same time.
Original comment by pekka.klarck
on 28 Nov 2013 at 10:52
I started working on this. I already did a pull and made a "flat merge"
(basically applied the merge as a patch without history.)
Next I will write some tests for this. We are planning a release now early next
week, so hopefully this will be ready by then.
Original comment by jussi.ao...@gmail.com
on 29 Nov 2013 at 12:23
This issue was updated by revision a0ed46356158.
Merged from
https://bitbucket.org/userzimmermann/robotframework/commits/branch/javakwargs
as a patch without history.
Original comment by jussi.ao...@gmail.com
on 29 Nov 2013 at 7:03
[deleted comment]
Tests in reb0436860814 and in r097da5a12760 there is new functionality to
verify that kwargs won't clash with the automatically assigned positional
argument names.
Original comment by jussi.ao...@gmail.com
on 29 Nov 2013 at 7:06
On a quite quick look this looks really good. Two comments/questions:
- Were test libraries used by the tests committed at some point? I didn't see
them in the revisions referenced above, but because tests are passing on CI
they probably have been committed.
- Should ArgumentSpec's `named_arguments` attribute be renamed to
`supports_named_arguments` or just `supports_named`? Now when you look at the
name and compare it to attributes `positional` or `defaults`, it's not that
clear that it is a Boolean and not, for example, a map of named args.
Original comment by pekka.klarck
on 30 Nov 2013 at 9:28
Ok, found the test library from revision 45444093184d.
Original comment by pekka.klarck
on 30 Nov 2013 at 9:29
This issue was updated by revision add0fa9f1d0d.
Change test library method to test that String arrays for varargs also before
kwargs
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 10:00
This issue was updated by revision 56d0c353c776.
named_arguments -> supports_named and some other small cleanups.
Original comment by pekka.klarck
on 2 Dec 2013 at 11:03
This issue was updated by revision 2f2a34d5d618.
Now libdoc handles also kwargs on java.
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 11:06
This issue was updated by revision 3206ecb960f5.
Document kwargs on java
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 12:16
This issue was updated by revision ad1f234c2ade.
Cleanup libdoc checks for class.
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 12:27
This issue was updated by revision a0e879d88727.
Cleaned up code in Libdoc.
Original comment by pekka.klarck
on 2 Dec 2013 at 1:02
This issue was updated by revision b95dc104ac47.
Update to docs that kwargs in java have to be a java.util.Map
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 1:13
This issue was updated by revision 29386a1891fb.
Fix bugs related to dynamic libraries with only one runKeyword implementation
and also fix calling a java keyword with a map as last argument with just a map.
Original comment by jussi.ao...@gmail.com
on 2 Dec 2013 at 8:38
This issue was updated by revision 7aa0ceef81e7.
Cleanup.
This issue has turned out to be a can of worms. Had we know all the corner
cases, this would have waited until the next release. We ought to be pretty
ready now, though.
Original comment by pekka.klarck
on 2 Dec 2013 at 9:31
This issue was updated by revision 6c1308ccc136.
We enabled passing a real Map to a Java keyword accepting Map as the last
argument earlier.
It also enabled using dicts in place of kwargs elsewhere, but now that is
disabled.
Original comment by pekka.klarck
on 2 Dec 2013 at 10:42
This issue was updated by revision b1640e3134a8.
Tests for dicts not generally being converted to kwargs were missing from the
previous commit.
Original comment by pekka.klarck
on 2 Dec 2013 at 11:10
This issue was updated by revision 51c59f214f39.
Changed JavaArgumentParser to accept only List/Map, not their sub types, as
varargs/kwargs. Jython doesn't coerce e.g. Python dicts to HashMap so using
anything else than List/Map would not generally work anyway.
Original comment by pekka.klarck
on 3 Dec 2013 at 12:59
This issue was updated by revision e3998a71c712.
Enanced docs. Definitely need a review/proof-reading with fresh eyes.
Original comment by pekka.klarck
on 3 Dec 2013 at 12:59
Original comment by jussi.ao...@gmail.com
on 3 Dec 2013 at 11:55
Original comment by pekka.klarck
on 3 Dec 2013 at 12:04
Acknowledgements in revision 2c825b7e4d4c.
Original comment by pekka.klarck
on 3 Dec 2013 at 12:07
Original issue reported on code.google.com by
user.stefanz
on 20 Nov 2013 at 3:34