grass.jupyter: Use weakref.finalize to manage TemporaryDirectory #2206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use UTF-8 as encoding for text files (JSON files here) in grass.benchmark result reading and writing. This explicitly specifies an encoding and thus it avoids unspecified-encoding warning from Pylint.
Although tempfile.TemporaryDirectory uses weakref.finalize to delete the directory and its documentation does mention deletion during garbage-collection and during interpreter shutdown, the intention in its source code is that a with statement or explicit cleanup call is made because otherwise it generates a warning about implicit deletion. Pylint correctly generates warning suggesting with statement (consider-using-with). In the future, we may consider implementing context-manager and cleanup method to the render classes, but even with that we need to stretch the existence of the temporary directory beyond one method, so the with statement is not applicable. For now, the classes are using only weakref.finalize which is sufficient for a notebook where the object life time is linked with the notebook's kernel life time which is relatively short. The Pylint warning is disabled and an explanatory comment is above.
This reverts commit 2282ecf.
wenzeslaus
changed the title
|
@chaedri Can you please look at this? |
chaedri
reviewed
Feb 14, 2022
There was a problem hiding this comment.
@wenzeslaus Looks good to me! I'll implement something similar in the space time dataset visualizations and interactive displays.
wenzeslaus
added a commit
that referenced
this pull request
Feb 15, 2022
This adds Pylint check job in a new workflow. It uses the current latest Pylint and latest Python. It uses Pylint to check for minimal supported Python version. The check runs for the grass package (python/grass) and for wxGUI (gui/wxpython). However, most of the files are ignored. Each of the directories has its own .pylintrc file. Pylint runs for grass.jupyter and grass.benchmark with only slightly modified configuration after fixes in targeted files (#2199, #2200, #2201, #2206). The approach for future compliance is that the other sub-packages should also eventually comply with default or almost default configuration. For wxGUI, many Pylint messages are disabled and numbers driving coding standard limits, such as max number of local variables, are greatly loosened. Many of the disabled messages show real errors, but ignoring these for now allowed for enabling the check right away at least for the least problematic directories. The approach for future compliance is that new directories can be added (removed from ignore paths) and new issues can be checked (removed from disable) when fixed. This compiles the code. Just testing Python functions from the grass package in the python directory does not require compiled code and package on path when started from the python directory, but checking other files requires packages on path. The config file is in the python directory because that's the focus of this PR. (In the future, GUI may have a different config file, e.g., due to different naming conventions. On the other hand, in an ideal case, the whole source code will use this file or even a more strict version of this file in the future.) Pylint should produce warning for disabled warnings which are not generated, although that seems to not be the case right now (useless-suppression). The only code change here is that Pylint disable comment needs to be on the same line or inside (). It needs to install also all optional Python dependencies because Pylint checks (can check unless disabled) all imports. This change is applied also to pytest workflow. Using workflow on settings to run on push only to main and release branches and semver tags. It uses strategy.matrix.include to get job-level config variables without using environmental variables.
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Oct 26, 2022
…Geo#2206) Although tempfile.TemporaryDirectory uses weakref.finalize to delete the directory and its documentation does mention deletion during garbage-collection and during interpreter shutdown, the intention in its source code is that a with statement is used or explicit cleanup call is made because otherwise it generates a warning about implicit deletion. Pylint correctly generates warning suggesting with statement (consider-using-with). In the future, we may consider implementing context-manager and cleanup method to the render classes, but even with that we need to stretch the existence of the temporary directory beyond one method, so the with statement is not applicable. For now, the classes are using only weakref.finalize which is sufficient for a notebook where the object life time is linked with the notebook's kernel life time which is relatively short. The Pylint warning is disabled and an explanatory comment is above.
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Oct 26, 2022
This adds Pylint check job in a new workflow. It uses the current latest Pylint and latest Python. It uses Pylint to check for minimal supported Python version. The check runs for the grass package (python/grass) and for wxGUI (gui/wxpython). However, most of the files are ignored. Each of the directories has its own .pylintrc file. Pylint runs for grass.jupyter and grass.benchmark with only slightly modified configuration after fixes in targeted files (OSGeo#2199, OSGeo#2200, OSGeo#2201, OSGeo#2206). The approach for future compliance is that the other sub-packages should also eventually comply with default or almost default configuration. For wxGUI, many Pylint messages are disabled and numbers driving coding standard limits, such as max number of local variables, are greatly loosened. Many of the disabled messages show real errors, but ignoring these for now allowed for enabling the check right away at least for the least problematic directories. The approach for future compliance is that new directories can be added (removed from ignore paths) and new issues can be checked (removed from disable) when fixed. This compiles the code. Just testing Python functions from the grass package in the python directory does not require compiled code and package on path when started from the python directory, but checking other files requires packages on path. The config file is in the python directory because that's the focus of this PR. (In the future, GUI may have a different config file, e.g., due to different naming conventions. On the other hand, in an ideal case, the whole source code will use this file or even a more strict version of this file in the future.) Pylint should produce warning for disabled warnings which are not generated, although that seems to not be the case right now (useless-suppression). The only code change here is that Pylint disable comment needs to be on the same line or inside (). It needs to install also all optional Python dependencies because Pylint checks (can check unless disabled) all imports. This change is applied also to pytest workflow. Using workflow on settings to run on push only to main and release branches and semver tags. It uses strategy.matrix.include to get job-level config variables without using environmental variables.
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Feb 17, 2023
…Geo#2206) Although tempfile.TemporaryDirectory uses weakref.finalize to delete the directory and its documentation does mention deletion during garbage-collection and during interpreter shutdown, the intention in its source code is that a with statement is used or explicit cleanup call is made because otherwise it generates a warning about implicit deletion. Pylint correctly generates warning suggesting with statement (consider-using-with). In the future, we may consider implementing context-manager and cleanup method to the render classes, but even with that we need to stretch the existence of the temporary directory beyond one method, so the with statement is not applicable. For now, the classes are using only weakref.finalize which is sufficient for a notebook where the object life time is linked with the notebook's kernel life time which is relatively short. The Pylint warning is disabled and an explanatory comment is above.
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Feb 17, 2023
This adds Pylint check job in a new workflow. It uses the current latest Pylint and latest Python. It uses Pylint to check for minimal supported Python version. The check runs for the grass package (python/grass) and for wxGUI (gui/wxpython). However, most of the files are ignored. Each of the directories has its own .pylintrc file. Pylint runs for grass.jupyter and grass.benchmark with only slightly modified configuration after fixes in targeted files (OSGeo#2199, OSGeo#2200, OSGeo#2201, OSGeo#2206). The approach for future compliance is that the other sub-packages should also eventually comply with default or almost default configuration. For wxGUI, many Pylint messages are disabled and numbers driving coding standard limits, such as max number of local variables, are greatly loosened. Many of the disabled messages show real errors, but ignoring these for now allowed for enabling the check right away at least for the least problematic directories. The approach for future compliance is that new directories can be added (removed from ignore paths) and new issues can be checked (removed from disable) when fixed. This compiles the code. Just testing Python functions from the grass package in the python directory does not require compiled code and package on path when started from the python directory, but checking other files requires packages on path. The config file is in the python directory because that's the focus of this PR. (In the future, GUI may have a different config file, e.g., due to different naming conventions. On the other hand, in an ideal case, the whole source code will use this file or even a more strict version of this file in the future.) Pylint should produce warning for disabled warnings which are not generated, although that seems to not be the case right now (useless-suppression). The only code change here is that Pylint disable comment needs to be on the same line or inside (). It needs to install also all optional Python dependencies because Pylint checks (can check unless disabled) all imports. This change is applied also to pytest workflow. Using workflow on settings to run on push only to main and release branches and semver tags. It uses strategy.matrix.include to get job-level config variables without using environmental variables.
neteler
pushed a commit
to nilason/grass
that referenced
this pull request
Nov 7, 2023
…Geo#2206) Although tempfile.TemporaryDirectory uses weakref.finalize to delete the directory and its documentation does mention deletion during garbage-collection and during interpreter shutdown, the intention in its source code is that a with statement is used or explicit cleanup call is made because otherwise it generates a warning about implicit deletion. Pylint correctly generates warning suggesting with statement (consider-using-with). In the future, we may consider implementing context-manager and cleanup method to the render classes, but even with that we need to stretch the existence of the temporary directory beyond one method, so the with statement is not applicable. For now, the classes are using only weakref.finalize which is sufficient for a notebook where the object life time is linked with the notebook's kernel life time which is relatively short. The Pylint warning is disabled and an explanatory comment is above.
neteler
pushed a commit
to nilason/grass
that referenced
this pull request
Nov 7, 2023
This adds Pylint check job in a new workflow. It uses the current latest Pylint and latest Python. It uses Pylint to check for minimal supported Python version. The check runs for the grass package (python/grass) and for wxGUI (gui/wxpython). However, most of the files are ignored. Each of the directories has its own .pylintrc file. Pylint runs for grass.jupyter and grass.benchmark with only slightly modified configuration after fixes in targeted files (OSGeo#2199, OSGeo#2200, OSGeo#2201, OSGeo#2206). The approach for future compliance is that the other sub-packages should also eventually comply with default or almost default configuration. For wxGUI, many Pylint messages are disabled and numbers driving coding standard limits, such as max number of local variables, are greatly loosened. Many of the disabled messages show real errors, but ignoring these for now allowed for enabling the check right away at least for the least problematic directories. The approach for future compliance is that new directories can be added (removed from ignore paths) and new issues can be checked (removed from disable) when fixed. This compiles the code. Just testing Python functions from the grass package in the python directory does not require compiled code and package on path when started from the python directory, but checking other files requires packages on path. The config file is in the python directory because that's the focus of this PR. (In the future, GUI may have a different config file, e.g., due to different naming conventions. On the other hand, in an ideal case, the whole source code will use this file or even a more strict version of this file in the future.) Pylint should produce warning for disabled warnings which are not generated, although that seems to not be the case right now (useless-suppression). The only code change here is that Pylint disable comment needs to be on the same line or inside (). It needs to install also all optional Python dependencies because Pylint checks (can check unless disabled) all imports. This change is applied also to pytest workflow. Using workflow on settings to run on push only to main and release branches and semver tags. It uses strategy.matrix.include to get job-level config variables without using environmental variables.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although tempfile.TemporaryDirectory uses weakref.finalize to delete the directory and its documentation does mention deletion during garbage-collection and during interpreter shutdown, the intention in its source code is that a with statement or explicit cleanup call is made because otherwise it generates a warning about implicit deletion. Pylint correctly generates warning suggesting with statement (consider-using-with).
In the future, we may consider implementing context-manager and cleanup method to the render classes, but even with that we need to stretch the existence of the temporary directory beyond one method, so the with statement is not applicable. For now, the classes are using only weakref.finalize which is sufficient for a notebook where the object life time is linked with the notebook's kernel life time which is relatively short. The Pylint warning is disabled and an explanatory comment is above.