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

[Rust] Refactors #11832

Merged
merged 11 commits into from
Apr 5, 2023
Merged

[Rust] Refactors #11832

merged 11 commits into from
Apr 5, 2023

Conversation

dev-ardi
Copy link
Contributor

Description

General refactors in the selenium manager

Motivation and Context

I was going to add a new functionality and found some code that could get updated.

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Changes

@CLAassistant
Copy link

CLAassistant commented Mar 29, 2023

CLA assistant check
All committers have signed the CLA.

@dev-ardi dev-ardi changed the title Refactors [Rust] Refactors Mar 29, 2023
@dev-ardi dev-ardi mentioned this pull request Mar 30, 2023
8 tasks
Copy link
Member

@bonigarcia bonigarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dev-ardi. Are you using cargo fmt to format the code in this PR?

@dev-ardi
Copy link
Contributor Author

dev-ardi commented Apr 3, 2023

Buenos días @bonigarcia,

Yes I am, that's the reason behind the commit ac551af
Are you using another formatter?

@bonigarcia
Copy link
Member

We also use cargo fmt, thanks!

@diemol
Copy link
Member

diemol commented Apr 4, 2023

@dev-ardi could you please sign the CLA and resolve the conflicts?

@dev-ardi
Copy link
Contributor Author

dev-ardi commented Apr 4, 2023

Done

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems tests are failing.

@dev-ardi
Copy link
Contributor Author

dev-ardi commented Apr 5, 2023

I have no idea how to merge these locally😞

@diemol
Copy link
Member

diemol commented Apr 5, 2023

You can use the GitHub UI for that...

image

https://github.com/SeleniumHQ/selenium/pull/11832/conflicts

@AutomatedTester
Copy link
Member

@dev-ardi hey, we have numerous chat services all interconnected if you're interested.

https://www.selenium.dev/support/

It might mean you get quicker answers as people might not always be watching GitHub notifications. We would love for you to join us and come hang out

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @dev-ardi!

Congratulations on your first contribution 🎉

@diemol diemol merged commit 4bf979b into SeleniumHQ:trunk Apr 5, 2023
9 checks passed
@dev-ardi dev-ardi deleted the refactors branch April 5, 2023 09:31
yashcho pushed a commit to yashcho/selenium that referenced this pull request Apr 7, 2023
* Refactor so that it's more concise

* cargo fmt

* I found it too verbose

* String interpolation

* Since the error message is the same now put it in the chain.

* Added never return type so I can remove the calls to exit()

---------

Co-authored-by: ogonzalez <ogonzalez@iconmm.com>
Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
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.

None yet

6 participants