Typehints etc: Not yet the docstrings, but the base to create docstrings #161
Conversation
signedav
changed the title
|
My goal was to invest a day in documentation of the library, like with docstrings etc. But in the end the docstrings are massively easier to generate when there are already proper typehints. And for working with the library they are at least as valuable. So these are at least the typehints of all the functions. |
Header format in proper py docsting style Header docstring in proper format headers of processing
…checked by real code monkey
signedav
changed the title
Type hints for iliwrapper Typehints for generator Type hints for utils Type hints for ilitoppingmaker
|
Same weird test result on latest release only... 🤔 |
gacarrillor
left a comment
gacarrillor
left a comment
There was a problem hiding this comment.
Since it's a massive PR, I must admit I didn't do an exhaustive check here. However, I spotted some minor issues that I addressed as comments.
Otherwise, very welcome changes!
| def db_or_schema_exists(self): | ||
| def db_or_schema_exists(self) -> bool: | ||
| """Whether the DB (for GPKG) or schema (for PG) exists or not.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
This was there to make it clear for subclasses which methods do need to be overridden.
Perhaps we can remove the line as you did and stick now to @abc.abstractmethod? See https://stackoverflow.com/a/44316506
There was a problem hiding this comment.
ah okay. But we are not consequent here. Why the db_or_schema_exists and the create_db_or_schema is abstract but not the metadata_exists is?
There was a problem hiding this comment.
What about having this on our to do list when going on with this:
- define @AbstractMethod wherever needed
- docstrings
There was a problem hiding this comment.
Sounds good. For the changes we could keep in mind that DBConnector should be abstract, so that nobody can use it directly, but should use subclasses instead.
| def get_db_connector(configuration): | ||
| def get_db_connector( | ||
| configuration: Ili2DbCommandConfiguration, | ||
| ) -> GPKGConnector | PGConnector | MssqlConnector: |
There was a problem hiding this comment.
Not sure what's the recommended approach here. Could it be DBConnector instead?
If we add Oracle to DBConector subclasses, updating this typing hint would be easily forgotten.
There was a problem hiding this comment.
One cannot really use a DbConnector, that's why I took the "real" types here.
Not sure if there will be another database added that quickly, but probably you are right.
There was a problem hiding this comment.
Yeah, both make sense. I'm fine with any approach here.
If time left
Generate docstrings templates according to type hints with AIFill up docstrings templates manually