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

Don't disable default features of form_urlencoded #1135

Closed
wants to merge 1 commit into from

Conversation

madsmtm
Copy link

@madsmtm madsmtm commented Mar 1, 2023

Description of change

In servo/rust-url#722, we would like to make the crate potentially not need alloc in the future, but to do so, we must now introduce a required alloc feature.

Since this project uses default-features = false on form_urlencoded (which previously did nothing), it will break once that change lands.

Links to any relevant issues

Also requires l1h3r/did_url#2 to land (since this crate depends on did_url).

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

N/A

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

In servo/rust-url#722, we would like to make the crate potentially not need `alloc` in the future, but to do so, we must now introduce a required `alloc` feature.

Since this crate uses `default-features = false` on `form_urlencoded` (which previously did nothing), it will break once that change lands.

Also requires l1h3r/did_url#2 to land (since this crate depends on `did_url`).
@eike-hass
Copy link
Collaborator

Hi @madsmtm 👋 Thank you for taking the time to open this PR and the consideration, we appreciate it!
Regarding l1h3r/did_url#2: We are considering forking the repository moving forward, since there has been no activity for the last years. In that case we would also adopt the default-features change in the fork.

@eike-hass eike-hass added No changelog Excludes PR from becoming part of any changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Mar 1, 2023
@madsmtm
Copy link
Author

madsmtm commented Mar 2, 2023

Regarding l1h3r/did_url#2: We are considering forking the repository moving forward, since there has been no activity for the last years. In that case we would also adopt the default-features change in the fork.

That's understandable - you're free to do so however you wish btw (e.g. I don't need the git attribution).

@eike-hass eike-hass self-requested a review March 3, 2023 09:04
@eike-hass
Copy link
Collaborator

Hi 👋 @madsmtm Would you be able to sign your commit?

@madsmtm
Copy link
Author

madsmtm commented Mar 3, 2023

Hmm, I quickly tried to do so but it seems it'll take me some more time to understand how git signing works first :/

I'd be fine with you / someone else just doing an empty rebase and pushing to my fork, or whatever works.

@eike-hass
Copy link
Collaborator

Hmm, I quickly tried to do so but it seems it'll take me some more time to understand how git signing works first :/

I'd be fine with you / someone else just doing an empty rebase and pushing to my fork, or whatever works.

No problem 👌 We will do the change in #1136 and close this PR.

@eike-hass eike-hass closed this Mar 3, 2023
@eike-hass eike-hass mentioned this pull request Mar 7, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No changelog Excludes PR from becoming part of any changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants