Skip to content

Add str.format_map method #1639

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

Merged
merged 1 commit into from
Dec 24, 2019

Conversation

doyshinda
Copy link
Contributor

Contributes to #190

Copy link
Member

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

This looks good; thanks for working in this! Could you also rebase onto master? The clippy tests are failing because there was a new rust version released, and the lint errors are fixed on master.

return Err(
vm.new_value_error("Format string contains positional fields".to_string())
);
}
Copy link
Member

Choose a reason for hiding this comment

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

You can merge this match arm and the previous one by using the | character: FormatPart::AutoSpec(_) | FormatPart::IndexSpec (_, _) => { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍



def test_format_map():
# The following tests were performed in Python 3.7.5:
Copy link
Member

Choose a reason for hiding this comment

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

Could you move these tests to tests/snippets/strings.py? And you don't need the separate test function; our tests right now are a lot less structured than most Python tests 😁

@coolreader18
Copy link
Member

Thanks again for contributing!

@coolreader18 coolreader18 merged commit 39ad03c into RustPython:master Dec 24, 2019
@doyshinda doyshinda deleted the impl_str_format_map branch December 24, 2019 00:29
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.

2 participants