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

Implement 'url!(..)' macro #137

Closed
wants to merge 1 commit into from
Closed

Implement 'url!(..)' macro #137

wants to merge 1 commit into from

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Nov 20, 2015

Fixes #136

Review on Reviewable

Fixes #136
@frewsxcv
Copy link
Member Author

frewsxcv commented Nov 20, 2015

I preemptively opened this because I have a couple questions:

  1. How should tests be structured/organized for compiler plugins?
  2. Should the url crate depend on url_plugin? Wouldn't that create a cyclic dependency?
plugin = true

[dependencies.url]
path = "../"

This comment has been minimized.

@Manishearth

Manishearth Nov 20, 2015

Member

I think for publishing to work you need to have a version dep here too. Not sure

@SimonSapin
Copy link
Member

SimonSapin commented Nov 20, 2015

To answer questions:

  1. I don’t think many tests are needed here, one to check that the plugin doesn’t panic and returns something sensible should be enough. It’d have to be in a separate crate so you can use the plugin. Cargo will compile a separate crate (run a separate rustc --test invocation) if you have a file named e.g. plugin/tests/smoke_test.rs, so you don’t need another Cargo.toml file.
  2. No it shouldn’t, why? Yes it would.

The code looks ok but (sorry I didn’t mention this earlier) I wonder if this plugin should be here rather than in Servo. We can move it back if someone asks, but in the meantime I don’t know if anyone else would use it. Meanwhile, this code uses unstable compiler internals (as plugins do) and rust-url is sort of expected to work on Rust Nightly, while Rust-in-Servo is updated less frequently.

To put it more bluntly, moving it to Servo is likely less future work for me :]

@Manishearth
Copy link
Member

Manishearth commented Nov 20, 2015

I wonder if this plugin should be here rather than in Servo

We don't need to ship this crate as part of rust-url. It can be published separately and used only by Servo.

@SimonSapin
Copy link
Member

SimonSapin commented Nov 20, 2015

I’m suggesting having this in the servo/servo repository rather than this one, where the Rust version used on CI is updated less frequently. This is not about publishing on crates.io.

@frewsxcv
Copy link
Member Author

frewsxcv commented Nov 20, 2015

Suggestions on where it should live in Servo?

@SimonSapin
Copy link
Member

SimonSapin commented Nov 20, 2015

The plugins crate?

frewsxcv added a commit to frewsxcv/servo that referenced this pull request Nov 20, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Nov 20, 2015

@frewsxcv frewsxcv closed this Nov 20, 2015
@frewsxcv frewsxcv deleted the url-macro branch Nov 20, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Nov 20, 2015
bors-servo added a commit to servo/servo that referenced this pull request Nov 20, 2015
Implement 'url!(..)' macro

servo/rust-url#136

servo/rust-url#137

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8622)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Nov 20, 2015
bors-servo added a commit to servo/servo that referenced this pull request Nov 21, 2015
Implement 'url!(..)' macro

servo/rust-url#136

servo/rust-url#137

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8622)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Nov 21, 2015
bors-servo added a commit to servo/servo that referenced this pull request Nov 21, 2015
Implement 'url!(..)' macro

servo/rust-url#136

servo/rust-url#137

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8622)
<!-- Reviewable:end -->
paulrouget added a commit to paulrouget/servo that referenced this pull request Nov 25, 2015
jrmuizel pushed a commit to jrmuizel/gecko-cinnabar that referenced this pull request Jun 12, 2017
…ugin); r=SimonSapin

servo/rust-url#136

servo/rust-url#137

Source-Repo: https://github.com/servo/servo
Source-Revision: ea690a2dff64d1cb4eb668473d62f1bbcb19f7c8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…ugin); r=SimonSapin

servo/rust-url#136

servo/rust-url#137

Source-Repo: https://github.com/servo/servo
Source-Revision: ea690a2dff64d1cb4eb668473d62f1bbcb19f7c8

UltraBlame original commit: 5bb91f3403d68562020f7bb5215820646e7cc5c6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…ugin); r=SimonSapin

servo/rust-url#136

servo/rust-url#137

Source-Repo: https://github.com/servo/servo
Source-Revision: ea690a2dff64d1cb4eb668473d62f1bbcb19f7c8

UltraBlame original commit: 5bb91f3403d68562020f7bb5215820646e7cc5c6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…ugin); r=SimonSapin

servo/rust-url#136

servo/rust-url#137

Source-Repo: https://github.com/servo/servo
Source-Revision: ea690a2dff64d1cb4eb668473d62f1bbcb19f7c8

UltraBlame original commit: 5bb91f3403d68562020f7bb5215820646e7cc5c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.