BF: Bug fixes and other improvement in SchemaBuilder.add_enum()
#341
Conversation
- Remove use of `kwargs` in setting `cls` if it's given as `ClassDefinition` - Correct detection of duplicating classes - Ensure `slots` is correct type when `use_attributes=True`
The example is outdated since `SchemaBuilder` no longer resides in `linkml.utils.schema_builder`
In this way, the slot definition in `slots` can be added to the class as an attribute, better than raising an exception
The bundling is actually not needed. The type annotation of `slots` already ensures that it is a list or `None`
Test the case of adding a class with a name that is the same as a class that already exists in the schema
… dict This can simplify the code
…tion` This time with detection of unsupported fields provided by `kwargs`
This makes the code easier to read
| ) | ||
|
|
||
|
|
||
| # === Tests for `SchemaBuilder.add_class` === |
There was a problem hiding this comment.
any reason for the # === comments - seems the docstrings should be sufficient?
There was a problem hiding this comment.
I used those to organize the tests into different sections, all tests for testing SchemaBuilder.add_class, all tests for testing SchemaBuilder.add_enum, and so on. I can organize the tests into classes like class TestAddClass and class TestAddEnum, but that will cost one indent. If you don't care about organizing the tests into different section, we can do it without the # === comment and the class organization. Let me know what you prefer.
There was a problem hiding this comment.
let's stick to standard pytest function-based tests. I think it's fine, we have similar markers in other larger test files, at some point we should have a consistent style back not a blocker at all!
There was a problem hiding this comment.
Do you prefer to have the # === lines removed?
This PR does the following to
SchemaBuilder.add_enum().Bug fixes (tests added):
PermissibleValueobjectsenum_defvalues of different typesOther improvements (no test needed):
Nonevalue inpermissible_valuesto an empty listNote: This PR is based on #338. To get the relevant commits of this PR, wait for #338 is merged to the main branch, and merge the main branch to this branch.