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

ARROW-241: Checking list-types in schema support raises an error #223

Merged
merged 7 commits into from
Jun 28, 2024
Merged

ARROW-241: Checking list-types in schema support raises an error #223

merged 7 commits into from
Jun 28, 2024

Conversation

lazargugleta
Copy link
Contributor

@lazargugleta lazargugleta commented Jun 26, 2024 •
edited by Jibola
Loading

@lazargugleta lazargugleta requested a review from a team as a code owner June 26, 2024 09:25
@lazargugleta lazargugleta mentioned this pull request Jun 26, 2024
Closed
@Jibola
Copy link
Collaborator

Jibola commented Jun 26, 2024

Thanks for supplying this fix @lazargugleta!

One last thing we'd need is for you to add a test case for this function. This ensures that we will not see this error again.

Jibola
Jibola requested changes Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

See above.

@Jibola Jibola changed the title List in schema support ARROW-241: Check Lists in schema support raises an error Jun 26, 2024
@Jibola Jibola changed the title ARROW-241: Check Lists in schema support raises an error ARROW-241: Checking list-types in schema support raises an error Jun 26, 2024
@lazargugleta
Copy link
Contributor Author

lazargugleta commented Jun 26, 2024 •
edited
Loading

@Jibola
Hey, thanks for the quick reply, I added the test
Hopefully in the correct place. Let me know If there is anything else needed.

ShaneHarvey
ShaneHarvey requested changes Jun 27, 2024
View reviewed changes
bindings/python/pymongoarrow/types.py Outdated Show resolved Hide resolved
bindings/python/pymongoarrow/types.py Outdated Show resolved Hide resolved
@ShaneHarvey
Copy link
Collaborator

LGTM, scheduling the tests.

@ShaneHarvey
Copy link
Collaborator

Why remove test_py_empty_list_raises?

@lazargugleta
Copy link
Contributor Author

Why remove test_py_empty_list_raises?

It tests the same case.
Only good thing about leaving it in is that is shows intended behaviour.
Should I leave it in?

@ShaneHarvey
Copy link
Collaborator

Yeah could you add it back? It's nice to have as an extra regression test. Also our pre-commit checks are failing. You will need to install and run:

pip install pre-commit
pre-commit run --all-files

@lazargugleta
Copy link
Contributor Author

lazargugleta commented Jun 27, 2024 •
edited
Loading

Yeah could you add it back?

Thanks, test added back + reformatted code according to formating

ShaneHarvey
ShaneHarvey approved these changes Jun 27, 2024
View reviewed changes
Copy link
Collaborator

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

Thanks!

@Jibola Jibola self-requested a review June 28, 2024 18:11
@ShaneHarvey ShaneHarvey merged commit ce4c1d1 into mongodb-labs:main Jun 28, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneHarvey ShaneHarvey ShaneHarvey approved these changes

@Jibola Jibola Jibola 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.
3 participants
@lazargugleta @Jibola @ShaneHarvey