Closed shbenzer closed 3 weeks ago
Name | Link |
---|---|
Latest commit | 15187a4a1023a46279322f7d85e1865b0c18ecfc |
Latest deploy log | https://app.netlify.com/sites/selenium-dev/deploys/66c37c04eeb5540008206408 |
Deploy Preview | https://deploy-preview-1873--selenium-dev.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
โฑ๏ธ Estimated effort to review: 2 ๐ต๐ตโชโชโช |
๐งช No relevant tests |
๐ No security concerns identified |
โก Key issues to review Inconsistent Comments The added comments are in English for all language versions, which may not be ideal for non-English documentation. Mixed Languages The added code comments are in English while the existing comments are in Portuguese, leading to inconsistency. |
Category | Suggestion | Score |
Best practice |
Use a more specific XPath to locate elements___ **Consider using a more specific XPath to find the 'ul' element. The current XPath'//ul' will find the first 'ul' element in the entire document, which might not be the intended behavior. Use a more specific XPath or consider using other locator strategies if possible.** [website_and_docs/content/documentation/webdriver/elements/finders.en.md [324-325]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1873/files#diff-e21fb1eabd6a7eaf605f8f733ad4e5406d5ad60908a37a6da9b2c363c021ceaeR324-R325) ```diff # Get first element of tag 'ul' -element = driver.find_element(By.XPATH, '//ul') +element = driver.find_element(By.XPATH, '//div[@id="content"]//ul') ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
Use a context manager for better resource management of the webdriver___ **Consider using a context manager (with statement) for the webdriver to ensure properresource management and automatic closing of the browser.** [website_and_docs/content/documentation/webdriver/elements/finders.en.md [309-310]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1873/files#diff-e21fb1eabd6a7eaf605f8f733ad4e5406d5ad60908a37a6da9b2c363c021ceaeR309-R310) ```diff -driver = webdriver.Chrome() -driver.get("https://www.example.com") +with webdriver.Chrome() as driver: + driver.get("https://www.example.com") + # Rest of the code... ``` Suggestion importance[1-10]: 7Why: | 7 | |
Consistency |
Apply the commented XPath best practice in the example code___ **The comment about adding "." to the beginning of the XPath is correct, but it's notbeing applied in the example. Update the XPath in the example to demonstrate this practice.** [website_and_docs/content/documentation/webdriver/elements/finders.en.md [327-328]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1873/files#diff-e21fb1eabd6a7eaf605f8f733ad4e5406d5ad60908a37a6da9b2c363c021ceaeR327-R328) ```diff # get children of tag 'ul' with tag 'li' -elements = driver.find_elements(By.XPATH, './/li') +elements = element.find_elements(By.XPATH, './/li') ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
Error handling |
Add error handling for cases when no elements are found___ **Consider adding error handling for the case when no elements are found. This willmake the code more robust and prevent potential exceptions.** [website_and_docs/content/documentation/webdriver/elements/finders.en.md [317-320]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1873/files#diff-e21fb1eabd6a7eaf605f8f733ad4e5406d5ad60908a37a6da9b2c363c021ceaeR317-R320) ```diff elements = element.find_elements(By.TAG_NAME, 'p') -for e in elements: - print(e.text) +if elements: + for e in elements: + print(e.text) +else: + print("No 'p' elements found") ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: | 7 |
@diemol this should be better
Implemented feature #1539 - adding example of xpath differentiating b/w .// and // in find_element
Description
extended example scripts in finders.XX.md for each translation
Motivation and Context
necessary to make documentation more comprehensive
Types of changes
Checklist