Implement user_data from custom keyword arguments #44
Conversation
… can be recreated from dictionary
arm61
changed the title
…@DataClass for any child class of Header
|
Instead of making the dataclass in @aglavic on a workflow note, it might make more sense for you to fork the |
…xtra keyword arguments
…ified unexpectedly
|
The modifications are ready to merge, @andyfaff and @arm61, please have a look. @andyfaff about the branches: The forking seems unnecessary extra effort on both the remote and local repository end. Isn't the point of a GIT repository to work on code together. |
|
Great, I'll take a look soon. The workflow I suggested is used by other mature projects, e.g. scipy/numpy. There's still only one feature branch, it's just done on contributors forks. The workflow makes it easier for projects as they grow. |
|
OK, I can try next time. I never forked a project so I'll have to learn how the workflow is done correctly. I think you are a bit optimistic to think that orsopy will grow to a large project with many contributors like numpy, though ;-) |
|
This looks good to me. |
|
@andyfaff shall we merge this? |
|
Sorry, on leave this week, so haven't looked. I can take a look next week. |
|
@aglavic if you rebase to |
|
@arm61 I'll wait until it's decided if this PR is accepted to avoid double effort. Although I don't understand why this branch can fail it's tests due to changes to another branch... |
|
The changes are upstream in the |
|
Ah, I understand. Thanks. Rebase didn't work out so well so I merged master into this branch. |
|
Yes, sorry, I was busy this week. I'll do it earlier next week. |
|
Looks good to me, so merging. Perhaps we should have an pre-commit hook that auto formats our codebase with an agreed set of defaults? |
|
@andyfaff |
|
If we use black autoformatter we can specify line length, etc. We can do it in a precommit hook to save frustration. |
|
There are benefits to having a standard style even if the style isn't to everyones preference (in particular for projects with contributors that aren't necessarily in close contact). |
| @@ -9,7 +9,9 @@ | |||
| from typing import Optional, Union, List, Tuple, get_args, get_origin, Literal | |||
| import typing | |||
| from inspect import isclass | |||
| from dataclasses import field, dataclass, fields | |||
| from dataclasses import field, dataclass, fields, \ | |||
There was a problem hiding this comment.
This seems to be a complete re-implementation of all the dataclasses internal functions.
There was a problem hiding this comment.
The PR does implement a custom init function, which appears to be necessary to allow non-standard arguments/entries when creating the ORSO object itself.
Does this create issues for the schema generation from the codebase? Does the approach have drawbacks?
This implementation allows the users to add additional "groups" to the header by supplying keyword arguments to Orso object.
The advantage over a user_data attribute are, that the recreation from the header works directly and that it will be forward/backward compatible if we adapt a new group to the standard.
We'll have to discuss if this should be pushed upward to Header to allow user data in any block, but that might make things less clean.
@andyfaff have a look if this works as you intended.