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

clippy: Map to an error type instead of using allowing result_unit_err in components/url #31834

Merged
merged 9 commits into from Mar 26, 2024

Conversation

ektuu
Copy link
Contributor

@ektuu ektuu commented Mar 22, 2024


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are related to clippy: fix warnings in /Components/url #31832
  • These changes do not require tests because they do not change functionality.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

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

Some points:

  • When you receive feedback in a PR, just update the old one, don't create a new one
  • The only difference from cd28798 is that you avoided inserting a space before #![deny(unsafe_code)], but you are still adding unnecessary newlines
  • Is there any reason to reorder the methods?
  • The template for pull requests has a list of things that you need to test, and check if they hold.
  • In particular, ./mach test-tidy is complaining: https://github.com/servo/servo/actions/runs/8395608656/job/22995237911?pr=31834
  • What are the warnings that you are trying to address?

@ektuu
Copy link
Contributor Author

ektuu commented Mar 23, 2024

Thankyou for your feedback. I will address the issues mentioned.

In the above PR, I used a custom error type to address the warnings of functions returning 'Result<_, ()>'.
Methods such as set_username, set_ip_host, set_password, to_file_path, and from_file_path have been modified to return a Result type with UrlError instead of (). Previously existing method definitions have been modified or removed to align with the new error handling strategy.

@ektuu
Copy link
Contributor Author

ektuu commented Mar 23, 2024

@Loirooriol
I've addressed the issues. can you please review?

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

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

What's the reason for reordering the methods?

@ektuu
Copy link
Contributor Author

ektuu commented Mar 23, 2024

Methods have been reordered to align with the new error handling strategy. The methods which use custom error UrlError have been put together to enhance readability of the code. Would you want me to reorder them ?

components/url/lib.rs Outdated Show resolved Hide resolved
@ektuu ektuu requested a review from Loirooriol March 24, 2024 11:13
components/url/lib.rs Outdated Show resolved Hide resolved
components/url/lib.rs Outdated Show resolved Hide resolved
@mrobinson mrobinson changed the title clippy: fix warnings in components/url clippy: Map to an error type instead of using allowing result_unit_err in components/url Mar 25, 2024
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@mrobinson
Copy link
Member

Looks like I was wrong here. This didn't go to the merge queue, because it is still waiting on a review from @Loirooriol, as he requested changes earlier.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

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

I share eerii's doubts about whether this is actually an improvement, but I'm fine with the change.

@mrobinson mrobinson added this pull request to the merge queue Mar 26, 2024
@mrobinson
Copy link
Member

I also agree that as it is, this isn't super useful, but it does prepare the code for better error handling in the future.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2024
@mrobinson
Copy link
Member

Looks like the failure was an infrastructure issue for the Android build:

Error: Exception in thread "main" java.net.ConnectException: Connection timed out

@mrobinson mrobinson added this pull request to the merge queue Mar 26, 2024
Merged via the queue into servo:main with commit 68b0be6 Mar 26, 2024
9 checks passed
ektuu added a commit to ektuu/servo that referenced this pull request Mar 28, 2024
…err` in `components/url` (servo#31834)

* clippy: fix warnings in components/url

* Fix code formatting issues in components/url/lib.rs

* Update components/url/lib.rs

Co-authored-by: eri <eri@inventati.org>

* made requested changes

---------

Co-authored-by: Ekta Siwach <ektasiwach@Ektas-MacBook-Air.local>
Co-authored-by: eri <eri@inventati.org>
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

5 participants