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

Propagate formatting parameters to each components for more types #421

Merged
merged 6 commits into from Apr 2, 2020

Conversation

@nical
Copy link
Collaborator

nical commented Apr 2, 2020

Following up on #419, ensure the Debug and Display implementations propagate formatting parameters for more of euclid's types.

This allows people to control, for example, the number of digits in the formatting syntax like println!("{:.3?}", rect);.

@nical
Copy link
Collaborator Author

nical commented Apr 2, 2020

r? @kvark

@kvark
kvark approved these changes Apr 2, 2020
Copy link
Member

kvark left a comment

sad

@@ -63,13 +63,21 @@ impl<T: Eq, U> Eq for Box2D<T, U> {}

impl<T: fmt::Debug, U> fmt::Debug for Box2D<T, U> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Box2D({:?}, {:?})", self.min, self.max)

This comment has been minimized.

@kvark

kvark Apr 2, 2020

Member

this should be raised at a lower level, i.e. the "proper" way to go here is unnecessary verbose :/
I could imagine something like write!(f, "Box2D({:~?}, {:~?})", self.min, self.max) that would propagate the f parameters automatically. Could you raise that issue upstream?

This comment has been minimized.

@nical

nical Apr 2, 2020

Author Collaborator

I agree with you, I think that the feature worth it though.

Could you raise that issue upstream?

I don't have the steam to champion an RFC for this at the moment, I'm asking around to see if I could get away requesting a feature like that directly as a github issue.

In the process @nox pointed out that for simple Debug formats we can do:

        f.debug_tuple("Box2D")
            .field(&self.min)
            .field(&self.max)
            .finish()

This is a bit nicer although it only works for Debug (no equivalent Display helpers), and won't work for Rect({size} at {origin}) as far as I can tell.

Would you prefer that I convert the debug impls that I can to this syntax or keep the more verbose but consistent way?

In any case I'd like to land this in some form of another without waiting for a potentital future version of the standard library.

This comment has been minimized.

@kvark

kvark Apr 2, 2020

Member

Converting the debug seems like an improvement to me, not a blocker to land this though.

Also, perhaps RFC isn't necessary the first step, but rather opening an issue saying that there is a problem here.

This comment has been minimized.

@nical

nical Apr 2, 2020

Author Collaborator

I converted the debug impls to use debug_tuple where applicable and filed rust-lang/rust#70710

@nical nical force-pushed the nical:formatting branch from 3e95ad6 to a61891b Apr 2, 2020
@nical
Copy link
Collaborator Author

nical commented Apr 2, 2020

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Apr 2, 2020

📌 Commit a61891b has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Apr 2, 2020

Testing commit a61891b with merge 0051e76...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 2, 2020

☀️ Test successful - checks-travis
Approved by: kvark
Pushing 0051e76 to master...

@bors-servo bors-servo merged commit 0051e76 into servo:master Apr 2, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@nox
Copy link
Member

nox commented Apr 2, 2020

Honestly I would just not implement Display.

@nical nical deleted the nical:formatting branch Apr 2, 2020
bors-servo added a commit that referenced this pull request Jul 12, 2020
(breaking change) Remove Display implementations

Fixes #445 See also #421.
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

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