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.

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

collection.first vs collection.first() ? #81

Open
yashaka opened this issue Mar 1, 2019 · 5 comments
Open

collection.first vs collection.first() ? #81

yashaka opened this issue Mar 1, 2019 · 5 comments

Comments

@yashaka
Copy link
Collaborator

yashaka commented Mar 1, 2019

So far (unfortunately we already released it in "stable" 1.0.0), we have implemented collection.first not as getter property but as method... (compare with element.parent whigh is implemented as property)

So... property or method? (we still can quick-fix 1.0.0 in 1.0.1 :D)

Here are things to take into account:

Option 1

    get first(): Element {
        return this.elementAt(0);
    }

    take(count?:number) {
        return this.collection.filteredBy(have.sizeLessThanOrEqual(count));
    }

// ... =>

await results.first.should(have.text('Selenium automates browsers'));
await results.take(2).should(have.texts('Selenium automates browsers', 'inclusion of the WebDriver API'))
    .then(find.first).click();

pros:

  • all.first without ()
  • two functions instead of one, more complicated

cons:

  • all.take - is not the same style as other similar methods (we ranamed all.find to all.elementAt by intent earlier... and now we break same rule with all.take)

VS Option 2

    first(count?: number): Element | Collection {
        if (count === undefined) {
            return this.elementAt(0)
        }
        return new Collection(new SlicedWebElementsLocator(0, count, this), this.configuration);
    }

// ... =>

await results.first().should(have.text('Selenium automates browsers'));
await results.first(2).should(have.texts('Selenium automates browsers', 'inclusion of the WebDriver API'))
    .then(find.first()).click();

pros:

  • all.first(2) is same user-oriented style as other methods (we renamed all.find to all.elementAt by intent earlier...)
    • the API is "less names", one name (first) for similar contexts, pretty readable...

cons:

  • all.first() is less consice (a bit more bulky) with () parentheses
    • in all.perform(something).then(find.first()) it's even more weird, especially taking into account that autocomplete will finished typed fi to first without (), and since all other "no arguments" methods are without parentheses in this context - the user will probably forget to add parentheses and will get a weird error, but good, at least, that he get it at compile time...
  • implementation is more magical (with if statement inside), and less KISS - harder for the user to understand its usage (yet it's not driving a space shuttle of course...)

I tend to implement option 2. Since we already have first as method (not property) in 1.0.0 and it is of more user-oriented style, and general API will be more consistent with it... I will tune the current implementation to first(count?: number), but let's finally define where we go...

@yashaka
Copy link
Collaborator Author

yashaka commented Mar 1, 2019

There is a problem with Option 2, the typescript usage has to be:

const collection = browser.all('li').first(2) as Collection;

We have to explicitly identify that the return type is Collection, not Element :(

Is there any way to make the usage nice? o_O

@yashaka
Copy link
Collaborator Author

yashaka commented Mar 1, 2019

have fixed last problem with:

    first(): Element;
    first(count: number): Collection;
    first(count?: number) {
        if (count === undefined) {
            return this.elementAt(0);
        }
        return new Collection(new SlicedWebElementsLocator(0, count, this), this.configuration);
    }

but here goes a question... "Will now autocomplere browser.all('li').first(2).HERE work same good in js as ts?`

yashaka added a commit that referenced this issue Mar 2, 2019
…sponding sliced collection and changed collection.first() to .first as property - this breaks API a bit 2. renamed condition.collection.hasSizeMoreThan to .hasSizeGreaterThan accordig to english grammar, also added .hasSizeGreaterThanOrEqual .hasSizeLessThanOrEqual and all corresponding have.* aliases; 3. removed commented SearchContext implementations and corresponding locator; 4. removed predicate parameter from element.followingSibling and make it property (again, breaks API a bit); 5. added changelog
@alex-popov-tech
Copy link
Collaborator

do you think that thinking about how some ide's will provide autocomplete is a reason to don't use such cool feature? I would leave it as you provided in last comment, since it's super cool, and we already have some features which won't support autocomplete (f.e. have.no and be.not) :)

@yashaka
Copy link
Collaborator Author

yashaka commented Jul 24, 2019

as I remember, autocomplete worked for have.no and have.not in Intellij Idea...
At least in the original implementation... are you sure it does not work now?

@alex-popov-tech
Copy link
Collaborator

hm, it really works :D
anyways, I would say it's editor responsibility to have proper autocompletion engine (f.e. tsserver works fine for js)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants