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

Remove Spanned and add Format trait to Punctuated #2269

Merged
merged 4 commits into from Jul 8, 2022

Conversation

eureka-cpu
Copy link
Contributor

This PR removes the Spanned trait in favor of the Format trait to allow dynamic formatting for type T and P on Punctuated.

@sezna
Copy link
Contributor

sezna commented Jul 7, 2022

would it be possible to add some more tests specific to the behavior of punctuated lists?

@eureka-cpu
Copy link
Contributor Author

would it be possible to add some more tests specific to the behavior of punctuated lists?

There's a few other examples that I need to add yes, but they require a good bit of refactoring so I am adding them on a separate PR.

@eureka-cpu
Copy link
Contributor Author

would it be possible to add some more tests specific to the behavior of punctuated lists?

There's a few other examples that I need to add yes, but they require a good bit of refactoring so I am adding them on a separate PR.

To add clarity to this, the refactoring is in regards to enum, struct and storage that also use Punctuated but at present they are handled inside of their respective format() functions instead of in Punctuated::format().

@eureka-cpu eureka-cpu enabled auto-merge (squash) July 8, 2022 03:43
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

lgtm

@eureka-cpu eureka-cpu merged commit 2a42019 into master Jul 8, 2022
@eureka-cpu eureka-cpu deleted the eureka-cpu/update-punctuated branch July 8, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants