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

Update index.rst #942

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update index.rst #942

wants to merge 1 commit into from

Conversation

schurma
Copy link

@schurma schurma commented Nov 9, 2021

The name='btnG' does not exist on "http://www.google.com" and a run of the sample code results in an IndexError exception. I propose that the button be accessed by value='Google Search' as this will make the sample code work and will be more resilient to change than continuing to use the often-changing Google element names.

name='btnG' does not exist on "http://www.google.com" and a run of the sample code results in an IndexError exception.  I propose that the button be accessed by value='Google Search' as this currently works and will probably be more resilient to change than the Google element names.
@jsfehler
Copy link
Collaborator

@schurma Your change wouldn't really make the code more resilient, not unless you know Google will never change the text in their submit button. It also introduces a breaking changes for anyone who uses google in a non-English language.

Finding elements by their text content should only be done when the text content is something that needs to be retrieved and evaluated. It's poor practice to use it for finding a button element and not the sort of thing I'd want displayed front and center in the documentation.

The code examples in the documentation should be more abstract. There's no way to control external websites and a google search example should be part of a longer tutorial, not part of the standard docs.

eg:

# Go to a website with a search input and submit button
# Then get the results of the search
with Browser('firefox') as b:
    b.visit('http://my.web.site/')
    b.find_by_css('#searchInput').fill('My Search Query')
    b.find_by_css('#searchSubmit').click()

    results = b.find_by_css('.results')
    if results[0].value == 'My Expected Result':
        print('Success!')
    else:
        print('Failure!')

@schurma
Copy link
Author

schurma commented Nov 22, 2021

That’s incorrect. This change will make the code more resilient. This will fix the broken script and prevent the breakage from recurring.

It isn’t true that this causes breakage for non-English users of Google. http://www.google.com defaults to English and the zope.testbrowser, geckodriver, and chromedriver engines used by Splinter all default to English. If someone knows how to change the language settings for those drivers then it’s incredibly unlikely that they’d be learning web automation for the first time using this sample script and then be perplexed why the “Google Search” text doesn’t match “Google [ & something clearly different]” text. That isn’t an instance of breaking changes. A breaking change is what happened to the Splinter example code after an ephemeral selector was used for a popular website.

It's correct that name selectors are often recommended over text selectors for a number of reasons, except that none of those reasons makes sense for the example. Google and other large companies know those recommendations and they craft those fields in order to break tools that crawl their websites. Popular websites have switched best practices with antipatterns for web automation.

The example doesn’t need to be abstract. It needs to work. It's embarrassing to recommend Splinter as the best web automation tool while the first example is broken. Most people won’t read any more docs if the first example is broken, especially if the broken example is used to explain the tool. If it’s important to maintain the remaining docs, then maybe the docs following the resilient example can begin it by asking the reader to get the name field of the button by viewing source. Then say, “For example, the name may equal btnk…”, then proceed from there. A sentence about the name selector being faster and better for dynamic text could be used without building a longer tutorial.

This change is the most resilient solution for the example. The language of the speaker doesn’t break the script and text value is the best selector. The text for that button hasn’t changed since 1998 and it’s reasonable to assume it won’t change in the future.

@nameloCmaS
Copy link
Contributor

This will be fixed in a different way with acceptance of PR #1255.

@schurma
Copy link
Author

schurma commented Mar 1, 2024 via email

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

Successfully merging this pull request may close these issues.

3 participants