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.

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

Use *Properties classes as argument types in Sprout #970

Open
martonvago opened this issue Jan 14, 2025 · 2 comments · May be fixed by #976
Open

Use *Properties classes as argument types in Sprout #970

martonvago opened this issue Jan 14, 2025 · 2 comments · May be fixed by #976
Assignees

Comments

@martonvago
Copy link
Contributor

martonvago commented Jan 14, 2025

[This only affects Sprout, not the checks package.]

Change all argument types for properties arguments from dict to the approperiate *Properties class.
Starting at top-level functions, we should change to using *Properties classes and use them as far down in the function stack as possible.

A natural point for transitioning to dict (so calling compact_dict on the properties) would be when the properties are fed to a check function or the write_json function.

@martonvago
Copy link
Contributor Author

I had a closer look at the implications of this and I think the check functions should actually be left as is, using dict.
This is because a key use case for check functions is loading a set of properties from the datapackage.json and checking that they meet the spec. These properties are loaded as dict. While it's not difficult to create a Properties object from a correct dict (i.e. a dict that has fields that correspond perfectly to the fields of a Properties class), the whole point is that at this stage we cannot guarantee that this dict is correct -- we have to check it first.

So I think we should still make this change, but leave check functions as is.

@signekb signekb self-assigned this Jan 16, 2025
@signekb signekb moved this from Todo to In Progress in Team project planning Jan 17, 2025
@signekb signekb linked a pull request Jan 20, 2025 that will close this issue
3 tasks
@signekb signekb moved this from In Progress to In Review in Team project planning Jan 20, 2025
@lwjohnst86
Copy link
Member

I think this is fine. I don't love the idea of just creating new object types to pass around, but I also come from a functional programming perspective rather than an OOP view.

As long as it is clear and simple, it works for me. I'll have to review the design docs to see how this fits with the change, but 👍 for me for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

3 participants