Skip to content
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.

Sign up for GitHub

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

Jump to bottom

865 aliases for kotlin #870

Merged
merged 7 commits into from Jan 12, 2019
Merged

865 aliases for kotlin #870

merged 7 commits into from Jan 12, 2019

Conversation

jkromski
Copy link
Contributor

@jkromski jkromski commented Nov 30, 2018 •
edited

Proposed changes

  • add find, findX, findAll, findAllX methods in Selenide class
  • deprecate Selenide.getElement* methods as they are the same as find* and they don't follow naming convention from SelenideDriver or SelenideElement

#865

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command - there are error not related with this change
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@rosolko
Copy link
Collaborator

rosolko commented Nov 30, 2018

@asolntsev @BorisOsipov From my point of view - it shouldn't be a part of selenide codebase, it should be implemented on client side if needed.

@asolntsev
Copy link
Member

@jkromski @rosolko @BorisOsipov

  1. I don't like findX and findAllX, but
  2. I like find and findAll

I think we can add methods find and findAll - they can be used in Kotlin, why not. It's better than s, S and other meaningless suggestions. Why not?

@vinogradoff
Copy link
Member

vinogradoff commented Dec 7, 2018 via email

@vinogradoff
Copy link
Member

vinogradoff commented Dec 7, 2018 via email

@jkromski
Copy link
Contributor Author

if you don't like find, what other names you got? Propose something please. In my opinion it should be verb in simple present form

@asolntsev I removed findX and findAllX

BTW. @vinogradoff Could you please don't add previous messages in yours, it creates noise. It is harder to read through what you exactly mean.

@vinogradoff
Copy link
Member

I don't like verbs here. This method defines how element will be searched. But it doesn't search.

We have element() or el() as other candidates...

@jkromski
Copy link
Contributor Author

looks good to me. It seems that it doesn't have to be a verb then.
so methods would look like:
element("a.link").shouldBe(visible);
and
elements("a.link").shouldBe(visible);
@asolntsev and @BorisOsipov what do you think?

@asolntsev
Copy link
Member

@jkromski I still don't see any problems with find and findAll.
element/elements is also a good option (except that it's too long).

Actually Selenide already has very similar methods getElement/getElements.
Does changing getElement to element really matter? I am not sure...
So, by the moment, I suggest to make a pause until we find some shorter name or agree on find.

@vinogradoff
Copy link
Member

vinogradoff commented Dec 12, 2018 via email

@jkromski
Copy link
Contributor Author

I think we need to look at it in bit wider scope and look at the SelenideElement and WebDriver methods too. Lets consider what we got in scenario like:

  • search for element
  • search for element in element

so for now we got names:

a. find("a.link") and find("div.container").shouldBe(visible).find("a.link")
b. element("a.link") and element("div.container").shouldBe(visible).find("a.link")
c. el("a.link") and el("div.container").shouldBe(visible).find("a.link")

Getting all arguments from @vinogradoff, @asolntsev and @BorisOsipov (but Boris is against whole idea) and me, my view is:
a. I favor this the most as is most consistent with existing methods in other classes (even that find and $ don't perform search, but this is the same in other classes)
b. I like first part but if you use it with sub-search then it is inconsistent. If we agree with this one, I would say that we would need to refactor names to follow: element("div.container").shouldBe(visible).descendant("a.link")
c. I don't really like shortcuts so no from my side

I would go with A now. I would go with B if we agree to refactor other names from find to descendant in SelenideElement

thoughts?

@vinogradoff
Copy link
Member

vinogradoff commented Dec 14, 2018 •
edited

Jacek, actually we don't have a use case for "search" an element, you need element either to interact with it or to assert some of his properties, but not just to "search".

so $(element).shouldBe(visible).$(subelement) -> is not complete.

Analogy. Let's say you have WYSIWYG palette in Delphi/VB/Visual C++ whatever IDE.
Then you drop widgets like Buttons, input fields, labels and so on your canvas, connecting them with some operations. Our browser view is analogy for canvas with widgets.

Then you code something. You probably need to interact with some widgets on the canvas (e.g. button1.Click()) or read their properties (e.g. inputField1.getText()).
There is though, no use case - like search button1 or inputField1 on the canvas. It is not necessary to search button1 and inputField1, they are declared and can be referenced when you want to use them.

I want to write tests the same way. Element declared with their locating methods/expressions, and then used for interacting or assertions. Luckily it is also the way Selenide works now.

That's why I don't want to have declaration to look like search operation.

@vinogradoff
Copy link
Member

vinogradoff commented Dec 14, 2018 •
edited

Then I take your examples, but with arguments right above. I just use element but it could be anything we agree on.

To declare an element use:
SelenideElement container=element("div.container");

To declare an element inside another element use:
SelenideElement link=element("div.container").element("a.link"); or
SelenideElement link=container.element("a.link");

To interact with previously defined element use:
link.click();

To assert properties of previously defined element use:
container.shouldBe(visible);

Does it make sense?

@jkromski
Copy link
Contributor Author

so you saying point b in my list should be:
element("div.container").element("a.link") instead element("div.container").descentant("a.link")
this requires refactoring, and I guess we want to have only one alias of $ so find would need to be removed. This change would need to go in major release, or now, we just deprecate find.

to sum up, we got:
a) find("div.container").find("a.link") + deprecate Selenide.getElement*
b) element("div.container").element("a.link") + deprecate Selenide.getElement*, SelenideElement.find*

but @asolntsev mentioned that it would better to have shorter name, elem? I don't like it and don't see better option:
https://www.thesaurus.com/browse/element?s=t

@jkromski
Copy link
Contributor Author

Can we agree on what to do over here?
I would do a option now. Option b we can move to other ticket and if we agree, do it for v5.2 or v6.0.

But I can do what you decide/agree

@asolntsev
Copy link
Member

@jkromski it seems that we don't have an "ideal" solution that would be better than current state.
So, it's probably good idea just to do nothing and stay where we are (until we get a new idea).

P.S. I found some symbols that are allowed in Kotlin. Probably we could use on of them instead of $?

        Œ(".active")
        Š(".active")
        ƒ(".active")
        œ(".active")
        Ÿ(".active")
        ª(".active")
        µ(".active")
        À(".active")

@jkromski
Copy link
Contributor Author

the symbols you gave I cannot find on the keyboard and I guess most of the people won't so it will be pain in the ... to use it in any language.
From what I'm guessing Selenide uses $ because it became some kind of a standard which came from JS frameworks, see https://api.jquery.com/jQuery and these frameworks have workaround for $ which is an alias in jQuery it is jQuery() and find()

@jkromski
Copy link
Contributor Author

jkromski commented Jan 9, 2019

can we make a decision over here, I would love to finish it and start something else. I guess people using Kotlin would like to have it released too

@asolntsev
Copy link
Member

asolntsev commented Jan 9, 2019 via email

@jkromski
Copy link
Contributor Author

To be honest, right now after doing some more tests, I think @vinogradoff proposition make a lot of sense as it fits scenarios which I mentioned before and if you would like to assure that element does not exists then you got:
element("a.link").shouldNot(exist)
which is very intuitive.

find("a.link").shouldNot(exist) does not make much sense here

@asolntsev
Copy link
Member

asolntsev commented Jan 10, 2019 via email

@jkromski
Copy link
Contributor Author

yup, collections looks good too. Shall I mark find* methods deprecated too or we leave it for later?

@rosolko
Copy link
Collaborator

rosolko commented Jan 11, 2019

@jkromski Just introduce element/elements and deprecate getElement/getElements

@vinogradoff
Copy link
Member

vinogradoff commented Jan 11, 2019 •
edited

I have another idea for collections.

Actually
elements("a.link").shouldHaveSize(3)
or
all("a.link").shouldHaveSize(3)
both not perfect

This looks right:
collection("a.link").shouldHaveSize(3)
or even
collectionOf("a.link").shouldHaveSize(3)

Downside: these are pretty long names.
Perhaps that helps to find more better ideas?

@jkromski
Copy link
Contributor Author

@vinogradoff personally I prefer element/elements. It was long discussion and I think at the end your proposition was the best. Pull request is ready to be re-reviewed

@vinogradoff
Copy link
Member

vinogradoff commented Jan 11, 2019 via email

@asolntsev asolntsev added this to the 5.1.1 milestone Jan 12, 2019
@asolntsev asolntsev merged commit 930c326 into selenide:master Jan 12, 2019
@asolntsev asolntsev modified the milestones: 5.1.1, 5.2.0 Jan 12, 2019
@asolntsev asolntsev mentioned this pull request Jan 12, 2019
Closed
@vinogradoff
Copy link
Member

let's also deprecate find?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asolntsev asolntsev asolntsev approved these changes

@BorisOsipov BorisOsipov Awaiting requested review from BorisOsipov

Assignees
No one assigned
Labels
feature has pr
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jkromski @rosolko @asolntsev @vinogradoff