Skip to content

Refactor v5#64

Merged
joloppo merged 31 commits into
v5from
v5-refactor
Sep 20, 2023
Merged

Refactor v5#64
joloppo merged 31 commits into
v5from
v5-refactor

Conversation

@joloppo
Copy link
Copy Markdown
Contributor

@joloppo joloppo commented Sep 5, 2023

Overview

  • Refactor according after discussions with @matteo-pallini last month.
  • I had to add the custom inject back in, rather than using .format as .format requires all variables to be formatted (could use std templates but that would change the syntax to $).
  • Validation now happens in the attached serde.

Comment thread dynamicio/io/resource.py Outdated
Comment thread dynamicio/io/resource.py
Comment thread dynamicio/io/postgres.py Outdated
Comment thread dynamicio/io/resource.py Outdated
Comment thread dynamicio/io/resource.py Outdated
Comment thread dynamicio/io/resource.py Outdated
Comment thread dynamicio/io/s3.py Outdated
Comment thread dynamicio/io/s3.py Outdated
Comment thread dynamicio/io/s3.py
Comment thread dynamicio/io/s3.py Outdated
Comment thread dynamicio/io/s3.py
Comment thread dynamicio/io/serde.py
Comment thread dynamicio/io/serde.py Outdated
Comment thread dynamicio/io/serde.py Outdated
Comment thread dynamicio/v5_migration/resource_templates.py Outdated
Comment thread tests/resource_tests/test_kafka.py
Comment thread dynamicio/io/postgres.py
Comment thread dynamicio/io/postgres.py
Comment thread dynamicio/io/postgres.py
Comment thread dynamicio/io/s3.py
Comment thread dynamicio/io/s3.py
Copy link
Copy Markdown

@matteo-pallini matteo-pallini left a comment

Choose a reason for hiding this comment

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

Left a bunch of comments. Most of them are nitpicks, so feel free to ignore them. Probably there is just 3 that I think it's definitely worth addressing:

Also, two other general comments:

  • I don't understand most of what is going on in the hdf.py module. So, if you want me to review that too it may be worth having a quick chat, and then I will need to spend a bit of time on that
  • Given that we spent quite a bit of time discussing about the refactoring design it may be worth also having someone else reviewing it. As, I am probably not in a good position to give you unbiased feedback on that

@joloppo
Copy link
Copy Markdown
Contributor Author

joloppo commented Sep 12, 2023

I've amended the PR - I left conversations I want you to have a look at open. Feel free to resolve them.

Copy link
Copy Markdown

@matteo-pallini matteo-pallini left a comment

Choose a reason for hiding this comment

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

It's a lot of changes, so maybe not everything will work perfectly on the first iteration but I think it's a step in the right direction. Thanks for doing this work, approved. As mentioned already, it may be worth having also another codeowner reviewing it, as it's a big change and I contributed to the design choices, so I am biased :-)

@joloppo joloppo merged commit ecb4ac4 into v5 Sep 20, 2023
@joloppo joloppo deleted the v5-refactor branch September 20, 2023 08:52
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