Skip to content

Conversation

@jorisvandenbossche
Copy link
Member

No description provided.

@github-actions
Copy link

Copy link
Contributor

@fsaintjacques fsaintjacques left a comment

Choose a reason for hiding this comment

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

One minor comments with the interface with regards to schema. Otherwise this is a good cleanup and will remove confusion with factory parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Schema shouldn't be optional, add a test if the user doesn't pass a schema to see the result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's not optional in practice: a bit below I check that the value is a Schema (and not None).
I did this here to give proper error messages when omitting one of those (there is some discussion about this in the original PR that added this: #6913 (comment)), because cython is otherwise generating very confusing error messages. I still need to create a minimal example to report to cython ..

But added a test for from_paths that covers this to ensure it is indeed checked.

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