[py] implementation of new feature-12760, setting service_args through getters/setters
#12767
Conversation
|
Also, can you rebase with trunk, I think we have conflicts |
sandeepsuryaprasad
force-pushed
the
service
branch
from
c95d6ac to
e7ae488
Compare
sandeepsuryaprasad
force-pushed
the
service
branch
from
e7ae488 to
e765368
Compare
|
I have cleaned it up.. |
sandeepsuryaprasad
changed the title
service_args though getters/settersservice_args through getters/setters
| def service_args(self, value): | ||
| if not isinstance(value, list): | ||
| raise TypeError("service args must be a list") | ||
| self._service_args.extend(value) |
There was a problem hiding this comment.
It might be just me, but the setter seems more like an append rather than replace/set the value.
Is this the intended behaviour? Setters for me usually replace the existing value
There was a problem hiding this comment.
every time when assignment happens to service_args I am not replacing the old args, rather I am updating the existing contents of the list.
e.g.
service = Service(service_args = ["--log", "debug"])
service.service_args = ["--port", 1234]
print(service.service_args)
# prints
["--log", "debug", "--port", 1234]
@titusfortner , should this be the intended behaviour? or should the old list be replaced with new one every time an assignment happens to service_args?
There was a problem hiding this comment.
I don't know the Python convention, but it does feel a little weird to append to something existing with an assignment operator. (They way Ruby does it is so much better 😉). I lean toward replacing instead of adding. @isaulv what do you think?
There was a problem hiding this comment.
This should definitely not be a setter as that creates the expectation that it is setting the value that you pass in. It is not intuitive to users that it will extend. We either need a different method name or we should see about closing this PR.
titusfortner
force-pushed
the
service
branch
from
8702e03 to
d05a92b
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #12767 +/- ##
==========================================
- Coverage 56.53% 55.88% -0.66%
==========================================
Files 86 86
Lines 5253 5316 +63
Branches 187 187
==========================================
+ Hits 2970 2971 +1
- Misses 2096 2158 +62
Partials 187 187
☔ View full report in Codecov by Sentry. |
|
Thanks @sandeepsuryaprasad! These seem to be 'placeholder' getter/setters for things at the moment which python typically doesn't really care about (its not as painful to add them in when required compared to other statically typed languages like java, but I don't really mind if we have them, can help with some validation around setting the service args |
| ) | ||
|
|
||
| @property | ||
| def service_args(self): |
There was a problem hiding this comment.
be nice to add typing to things we are adding, feel free to ignore tho as theres plenty of general typing work to do
There was a problem hiding this comment.
@symonk thanks for your review comments. I have updated the code.
sandeepsuryaprasad
force-pushed
the
service
branch
from
d05a92b to
093e3df
Compare
|
Sorry this one has languished, can you de-conflict it and I'll review again? Thanks! |
|
I am closing this because there are several file conflicts, but also, there was no more activity from the OP. |
|
this was done in #15889 |
8 participants
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This PR is an implementation of new feature request #12760, for setting
service_argsnot only through constructor, but also throughsetter/gettermethods using properties.Motivation and Context
New feature request 12760
Types of changes
Checklist