austin/match-ts-parity #32
Conversation
| if self.is_executed: | ||
| raise ValidationError( | ||
| "This workflow has already been executed. Create a new workflow builder for additional operations." | ||
| ) |
There was a problem hiding this comment.
Can we really only execute the workflow once? It'd be nice to have workflows being more flexible and executed multipe times, for example in the original input PDF changes, you can execute the workflow again.
I believe this is how the TS version was designed.
There was a problem hiding this comment.
Oh I see how this is used now. :S I still feel we should be able to update a workflow even after it is run. As see them as blueprints, they are not the executors, but just guidelines on how to run. That means we should be able update those blueprints at a later date.
There was a problem hiding this comment.
I think this was also implemented in the TypeScript version.
https://github.com/PSPDFKit-labs/nutrient-dws-client-typescript/blob/626f7fe652867e16171b65648c66f6444b61b978/src/builders/workflow.ts#L362
Regarding the design itself, I think the reason it was designed this way is because the documentation (our documentation) is unclear whether the workflow clean up after running only clean up the file parts or also the instructions
| self.assets.clear() | ||
| self.asset_index = 0 | ||
| self.current_step = 0 | ||
| self.is_executed = True |
There was a problem hiding this comment.
Similar thoughts about "blueprints" and "executors"
There was a problem hiding this comment.
I think the blueprint-executor design is not compatible with the staged builder design since the stage builder doesn't make it clear which part in each stage is static (blueprint) and dynamic (executors)
|
|
||
| def workflow( | ||
| api_key: str | Callable[[], str], | ||
| base_url: str | None = None, |
There was a problem hiding this comment.
We could default this to https://api.nutrient.io/ for simplicity.
There was a problem hiding this comment.
I think keeping it as None is better, This pattern also appears here
And if we change it we will add another location where the default URL is hardcoded, versus rightnow we only hard code it in one location
| 5. Open a Pull Request | ||
| ## Python Version Support | ||
|
|
||
| This library supports Python 3.10 and higher. The async-first design requires modern Python features for optimal performance and type safety. |
There was a problem hiding this comment.
Does it really need 3.10? I know the default on macos is 3.9, it'd be nice to support standard distributions out there.
There was a problem hiding this comment.
We are using match statement and typing syntax (|) that is introduced in 3.10
Match syntax here:
3.9 is also near end-of-life so I think it's okay if we drop it
Brought the Python library to full feature parity with the TypeScript client. Updated documentations and GitHub Workflow. Up the version number to 2.0.0 due to breaking features