Skip to content
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

Lint workflow for python import reordering #6613

Closed

Conversation

jamesobutler
Copy link
Contributor

This closes #6272 by use of https://github.com/asottile/reorder_python_imports. It's style is similar to black philosophy of trying to reduce merge conflicts (https://github.com/asottile/reorder_python_imports#why-this-style).

Note that the original describes a solution using flake8-import-order, but upon reviewing the repo https://github.com/PyCQA/flake8-import-order and specifically https://github.com/PyCQA/flake8-import-order/issues/163, there appears to be discussion about archiving the project or letting someone else take it over. Due to this uncertainty I went with https://github.com/asottile/reorder_python_imports which was mentioned as a python reordering hooks at https://pre-commit.com/hooks.html.

This style is not conforming to the current (see below) such as one package per line with multiple functions allowed. Something to consider.

- Imports
- Grouped in order of scope/commonallity
- Standard library imports
- Related third party imports
- Local apps/library specific imports
- Slicer application imports and local/module imports may be grouped independently.
- One package per line (with or without multiple function/module/class imports from the package)

Once merged STYLE: Apply reorder-python-imports changes could be a good candidate for inclusion in https://github.com/Slicer/Slicer/blob/main/.git-blame-ignore-revs.

@jamesobutler
Copy link
Contributor Author

Still need to test locally to confirm that any order changing hasn't broken anything.

@jcfr
Copy link
Member

jcfr commented Oct 21, 2022

Thanks for working on this 🙏 Having a way to systematically enforce the orders makes sense.

Alphabetical re-ordering within a group of imports makes sense.

That said, I am wondering is there is a way to configure the tool to:

  • keep the grouping stdlib/package/Slicer
  • keep the from package import (a, b, c) across multiple lines without changing it to from package import a, ...

I will further review on Monday when back.

Comment on lines -1 to +4
from .SegmentStatisticsPluginBase import *

from .ClosedSurfaceSegmentStatisticsPlugin import *
from .LabelmapSegmentStatisticsPlugin import *
from .ScalarVolumeSegmentStatisticsPlugin import *
from .SegmentStatisticsPluginBase import *
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some issues seemingly going on related to SegmentStatistics ordering

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\S5R\python-install\Lib\imp.py", line 169, in load_source
    module = _exec(spec, sys.modules[name])
  File "<frozen importlib._bootstrap>", line 613, in _exec
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "C:/S5R/Slicer-build/lib/Slicer-5.1/qt-scripted-modules/SegmentStatistics.py", line 7, in <module>
    from SegmentStatisticsPlugins import *
  File "C:\S5R\Slicer-build\lib\Slicer-5.1\qt-scripted-modules\SegmentStatisticsPlugins\__init__.py", line 1, in <module>
    from .ClosedSurfaceSegmentStatisticsPlugin import *
  File "C:\S5R\Slicer-build\lib\Slicer-5.1\qt-scripted-modules\SegmentStatisticsPlugins\ClosedSurfaceSegmentStatisticsPlugin.py", line 6, in <module>
    class ClosedSurfaceSegmentStatisticsPlugin(SegmentStatisticsPluginBase):
TypeError: module() takes at most 2 arguments (3 given)
loadSourceAsModule - Failed to load file "C:/S5R/Slicer-build/lib/Slicer-5.1/qt-scripted-modules/SegmentStatistics.py"  as module "SegmentStatistics" !
Fail to instantiate module  "SegmentStatistics"

Comment on lines -5 to +7
import vtk
import qt

import slicer
import vtk
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some issues seemingly go on related to ordering

Traceback (most recent call last):
  File "C:\S5R\Slicer-build\lib\Slicer-5.1\qt-scripted-modules\SegmentEditorEffects\SegmentEditorThresholdEffect.py", line 40, in __init__
    self.setupPreviewDisplay()
  File "C:\S5R\Slicer-build\lib\Slicer-5.1\qt-scripted-modules\SegmentEditorEffects\SegmentEditorThresholdEffect.py", line 648, in setupPreviewDisplay
    if not self.scriptedEffect.segmentationDisplayableInView(sliceWidget.mrmlSliceNode()):
TypeError: method requires a VTK object
qSlicerPythonCppAPI::instantiateClass  - [ "SegmentEditorThresholdEffect" ] - Failed to instantiate scripted pythonqt class "SegmentEditorThresholdEffect" 0x23b3d977090

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ordering issue is likely what was described in the following comment:

#6484 (comment)

you need to import vtk before you instantiate VTK-based classes.

Comment on lines -1 to +2
from .SliceViewAnnotations import SliceAnnotations
from .DataProbeUtil import DataProbeUtil
from .SliceViewAnnotations import SliceAnnotations
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some issues seemingly going on related to ordering for DataProbe

Traceback (most recent call last):
  File "C:/S5R/Slicer-build/lib/Slicer-5.1/qt-scripted-modules/DataProbe.py", line 55, in addView
    self.infoWidget = DataProbeInfoWidget(parent)
  File "C:/S5R/Slicer-build/lib/Slicer-5.1/qt-scripted-modules/DataProbe.py", line 94, in __init__
    self._createSmall()
  File "C:/S5R/Slicer-build/lib/Slicer-5.1/qt-scripted-modules/DataProbe.py", line 402, in _createSmall
    self.sliceAnnotations = DataProbeLib.SliceAnnotations()
  File "C:\S5R\Slicer-build\lib\Slicer-5.1\qt-scripted-modules\DataProbeLib\SliceViewAnnotations.py", line 24, in __init__
    self.dataProbeUtil = DataProbeUtil.DataProbeUtil()
AttributeError: type object 'DataProbeUtil' has no attribute 'DataProbeUtil'

@jcfr
Copy link
Member

jcfr commented Oct 21, 2022

Re: ordering issue

These issues seem familiar, looking at the history of some of the script may give insight as I encountered similar ones in the past.

@jamesobutler
Copy link
Contributor Author

Closing stale PR. Can use as reference for future work towards closing the related issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Improve lint workflow to verify python import ordering
2 participants