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

feat: add logic to convert severity string to uppercase #744

Merged
merged 7 commits into from
Apr 21, 2023
Merged

feat: add logic to convert severity string to uppercase #744

merged 7 commits into from
Apr 21, 2023

Conversation

ymotongpoo
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #743

@ymotongpoo ymotongpoo requested review from a team as code owners April 13, 2023 07:22
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: logging Issues related to the googleapis/python-logging API. labels Apr 13, 2023
@ymotongpoo
Copy link
Contributor Author

the reason why I skipped isupper() check is anyway it requires upper() in the case of lower/mixed case. Though it may cause some extra overhead for PyMem_Malloc, it should not that large so I simplified the process.

@daniel-sanche
Copy link
Contributor

This looks great! Would you mind adding a test for this too though, since the bug must have slipped through with the existing tests?

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: xs Pull request size is extra small. labels Apr 17, 2023
@ymotongpoo
Copy link
Contributor Author

@daniel-sanche I added a small unit test to check if Batch#commit() can properly convert severity strings.

daniel-sanche
daniel-sanche approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

This was referenced Apr 19, 2023
Closed
Closed
@daniel-sanche daniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 19, 2023
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 19, 2023
@daniel-sanche
Copy link
Contributor

You can ignore the environment test failures. It looks like I need to upgrade the pandas version there, I'll address that in a separate PR

It looks like there's a lint issue on the Kokoro check before this can be merged though

@ymotongpoo
Copy link
Contributor Author

I just ran nox and the result was the following (my local doesn't have Python 3.7, so unit-3.7 didn't run):

nox > Session docs was successful.
nox > Ran multiple sessions:
nox > * unit-3.7: failed
nox > * unit-3.8: success
nox > * unit-3.9: success
nox > * unit-3.10: success
nox > * unit-3.11: success
nox > * system-3.8: success
nox > * cover: success
nox > * lint: success
nox > * lint_setup_py: success
nox > * blacken: success
nox > * docs: success

I hope this fix works.

@daniel-sanche daniel-sanche added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 20, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 20, 2023
@daniel-sanche daniel-sanche merged commit c1c8ce1 into googleapis:main Apr 21, 2023
@daniel-sanche
Copy link
Contributor

looks great, thanks!

@release-please release-please bot mentioned this pull request Apr 21, 2023
Merged
@meredithslota meredithslota mentioned this pull request May 10, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-sanche daniel-sanche daniel-sanche approved these changes

Assignees

@losalex losalex

Labels
api: logging Issues related to the googleapis/python-logging API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert severity string to uppercase in Batch#log_struct
4 participants
@ymotongpoo @daniel-sanche @yoshi-kokoro @losalex