-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: Add a logger for all the vtk filter of geos-processing and all the plugin of geos-pv #178
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
base: main
Are you sure you want to change the base?
refactor: Add a logger for all the vtk filter of geos-processing and all the plugin of geos-pv #178
Conversation
…ilterAndPlugins Update to the last version of the main
…ilterAndPlugins Update to the last version of the main
…ilterAndPlugins Update to the last version of the main
jafranc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the try-except block there is some refactoring
|
|
||
|
|
||
| class RaiseMergeBlocks( TestCase ): | ||
| """Test failure on empty multiBlockDataSet.""" | ||
|
|
||
| def test_TypeError( self ) -> None: | ||
| """Test raise of TypeError.""" | ||
| multiBlockDataset = vtkMultiBlockDataSet() # should fail on empty data | ||
| if Version( vtk.__version__ ) < Version( "9.5" ): | ||
| with pytest.raises( VTKError ): | ||
| multiblockModifiers.mergeBlocks( multiBlockDataset, True ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why in this PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I changed the filter MergeBlockEnhanced, its test needed to be change to, this test was usless with the new implementation of the filter so I move it to the test of the function mergeBlock
| f"The { self.piece } attributes { attributesAlreadyInMeshTo } are already present in the final mesh." ) | ||
| self.logger.error( f"The filter { self.logger.name } failed." ) | ||
| return False | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure of try except and raise in the try.
Raising here is fine but capturing should be done in the caller or caller's caller tbf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this pr I wanted to add or update the logger. To do that I used the same scheme try/except in all the plugins and filters. But your are right, the filtre may not use try/except. To keep "small" and "unitary" pr, I think it can be done in another pr (see pr #185 )
| self.logger.error( f"The new attribute { self.newAttributeName } has not been added." ) | ||
| self.logger.error( f"The filter { self.logger.name } failed." ) | ||
| return False | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
In the context of the code refactoring, this pr aims at creating a logger for all the vtk filters and all the ParaView plugins inplemented if absent.
This pr follows the pr #181.
This pr will not uniformize the output message of these logger. It will be done in a onther pr following the issue #184