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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only Metadata<'static> works, remove generic #17

Closed
wants to merge 1 commit into from
Closed

Only Metadata<'static> works, remove generic #17

wants to merge 1 commit into from

Conversation

remram44
Copy link

Choose one: is this a 馃悰 bug fix

Metadata<'a> is wrong, since only a==static will compile. This changes it for clarity.

Checklist

  • tests pass (still no tests)
  • tests and/or benchmarks are included
  • documentation is changed or added

Context

N/A

Semver Changes

Changed Metadata is technically pub, though users of this library are unlikely to provide their own.

Copy link
Collaborator

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This does look good to me; but not too experienced with lifetimes - so going to wait for another review before merging. Thanks a lot already though!

@killercup
Copy link
Collaborator

I mentioned this in @spacekookie's latest PR (#15).

@yoshuawuyts, let's talk about this a bit. Are all these fields actually always present? I'd make these fields private (of some type) and provide getters that return Option<&str> (and never ever return Some("")). (Also, I'd probably sprinkle Cows all over the code base, but that's another story.)

@yoshuawuyts
Copy link
Collaborator

Are all these fields actually always present?

Yeah, probably not. Good call to make it optional. Also allows us to create better reports, cleaning up #3.

I'd make these fields private (of some type) and provide getters that return Option<&str> (and never ever return Some("")).

Sounds reasonable. Most of the stringly typed API stuff was just to get something working. Agree this is cleaner.

(Also, I'd probably sprinkle Cows all over the code base, but that's another story.)

Cool! - never used it before; curious to learn more!

@remram44
Copy link
Author

remram44 commented Apr 17, 2018

Are all these fields actually always present? I'd make these fields private (of some type) and provide getters that return Option<&str> (and never ever return Some("")). (Also, I'd probably sprinkle Cows all over the code base, but that's another story.)

Funny you should mention this. I tried some of that too (aee0194) but it ended up kind of complicated.

I don't think there is much point for the user to override some of those fields. It is simply more convenient and more powerful to provide a new message for the terminal as a whole...

Re: making fields private, remember that the Metadata struct is built from the user's crate via the macro (though a constructor could be provided).

@killercup killercup mentioned this pull request May 28, 2018
3 tasks
@spacekookie
Copy link
Collaborator

This was implemented in the end by #32

@remram44 remram44 deleted the static-lifetime branch September 26, 2019 20:49
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

4 participants