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.

Sign up for GitHub

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

Jump to bottom

Replace CslStringList From implementation with TryFrom #362

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Replace CslStringList From implementation with TryFrom #362

merged 1 commit into from
Jan 30, 2023

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jan 27, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This isn't that ugly, and I think it's better than panicking in a From implementation.

@lnicola lnicola force-pushed the csl-string-list-try-from branch from c69c516 to 21efc75 Compare January 27, 2023 18:26
@lnicola
Copy link
Member Author

lnicola commented Jan 27, 2023

r? @metasim

metasim
metasim approved these changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@metasim metasim left a comment

Choose a reason for hiding this comment

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

Looks like 1.67 came with new clippy lints...

src/cpl.rs
///
/// assert_eq!(count_opts(()), 0);
/// ```
impl From<()> for CslStringList {
Copy link
Contributor

Choose a reason for hiding this comment

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

😢

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't so bad, is it? I mean, it's from(&[]) vs. from(())..

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I'm just being silly. I was bothered by the verbosity of src.make_valid(&CslStringList::new()) (vs. src.make_valid(())), but fell into being overly clever and not really common/idiomatic. I understand why you got rid of it.

src/vector/geometry.rs Show resolved Hide resolved
@metasim
Copy link
Contributor

metasim commented Jan 27, 2023

bors d+

@bors
Copy link
Contributor

bors bot commented Jan 27, 2023

✌️ lnicola can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member Author

lnicola commented Jan 27, 2023

There's another PR for the lints.

@lnicola lnicola force-pushed the csl-string-list-try-from branch from 21efc75 to a68aba7 Compare January 28, 2023 17:24
@lnicola
Copy link
Member Author

lnicola commented Jan 28, 2023

Rebased over #363.

@lnicola
Copy link
Member Author

lnicola commented Jan 30, 2023

bors d=metasim

@bors
Copy link
Contributor

bors bot commented Jan 30, 2023

✌️ metasim can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member Author

lnicola commented Jan 30, 2023

bors r=metasim

@bors
Copy link
Contributor

bors bot commented Jan 30, 2023

Build succeeded:

@bors bors bot merged commit bd8f877 into georust:master Jan 30, 2023
@lnicola lnicola deleted the csl-string-list-try-from branch January 30, 2023 18:56
@metasim metasim mentioned this pull request May 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metasim metasim metasim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lnicola @metasim