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.

Sign up for GitHub

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

Jump to bottom

style: address flake8 e402 warning in the codebase #3354

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

style: address flake8 e402 warning in the codebase #3354

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40b60a4
style: address flake8 e402 warning in the codebase
Sharansrj567 Jan 11, 2024
c8ab468
Merge branch 'main' into fix-flask8-e402-warning
Sharansrj567 Jan 11, 2024
ea17773
refactor: bring modules at high level solving flake8 E402 warning
Sharansrj567 Jan 12, 2024
9a36b7e
Merge branch 'main' into fix-flask8-e402-warning
Sharansrj567 Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions gui/wxpython/core/gcmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,39 @@
"""

import os
import wx
import sys
import time
import errno
import signal
import traceback
import locale
import traceback
import subprocess
from threading import Thread
import wx

is_mswindows = sys.platform == "win32"
if is_mswindows:
from win32file import ReadFile, WriteFile
from win32pipe import PeekNamedPipe
import msvcrt
else:
import select
import fcntl

from threading import Thread
from core.debug import Debug
from core.globalvar import SCT_EXT

from grass.script import core as grass
Copy link
Member

Choose a reason for hiding this comment

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

Even if that's not from this PR, this doesn't sound right, and quite ambiguous (the alias's name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed that across our codebase, there is a consistent use of the alias grass for the core module. Given that this pattern is present in around 51 files, it might be worth discussing whether we want to maintain this convention or consider a more descriptive alias.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this was introduced when transitioning form a single-file package called grass(.script?) to multiple files (modules). Functions in original grass or something like that were moved core and grass or grass.script got new functions. grass.script.core then became what grass once was. These imports were the least intrusive change, so it was implemented that way. This was in the pre-v7-times on Subversion trunk, but it was propagated sufficiently through the code base at the time of v7 release that it became a standard for v7. We came up with grass.script as gs as a better practice, but didn't go back (yet) to replace all occurrences of core as grass.

from grass.script.utils import decode, encode

is_mswindows = sys.platform == "win32"


def platform_specific_imports():
if is_mswindows:
global ReadFile, WriteFile, PeekNamedPipe, msvcrt
from win32file import ReadFile, WriteFile
from win32pipe import PeekNamedPipe
import msvcrt
else:
global select, fcntl
import select
import fcntl


platform_specific_imports()


def DecodeString(string):
"""Decode string using system encoding
Expand Down
8 changes: 3 additions & 5 deletions gui/wxpython/core/watchdog.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file I'm ok with.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

import os
import time
import wx
from wx.lib.newevent import NewEvent
from grass.script import core as grass

watchdog_used = True
try:
Expand All @@ -32,11 +35,6 @@
PatternMatchingEventHandler = object
FileSystemEventHandler = object

import wx
from wx.lib.newevent import NewEvent

from grass.script import core as grass

updateMapset, EVT_UPDATE_MAPSET = NewEvent()
currentMapsetChanged, EVT_CURRENT_MAPSET_CHANGED = NewEvent()

Expand Down
4 changes: 2 additions & 2 deletions gui/wxpython/datacatalog/tree.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file I'm ok with.

Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@
"""
import os
import re
import wx
import copy
from multiprocessing import Process, Queue, cpu_count

import wx
import grass.script as gscript

from core.gcmd import RunCommand, GError, GMessage
from core.utils import GetListOfLocations
Expand Down Expand Up @@ -63,7 +64,6 @@

from grass.pydispatch.signal import Signal

import grass.script as gscript
from grass.script import gisenv
from grass.grassdb.data import map_exists
from grass.grassdb.checks import (
Expand Down
4 changes: 2 additions & 2 deletions gui/wxpython/docs/wxgui_sphinx/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import string
from shutil import copy

from grass.script import core

# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
Expand All @@ -25,8 +27,6 @@
0, os.path.abspath(os.path.join(os.environ["GISBASE"], "etc", "python", "grass"))
)

from grass.script import core

Comment on lines 27 to -29
Copy link
Member

Choose a reason for hiding this comment

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

This might not behave successfully. Is there a way to know if it imports well, and not only wrapped inside a grass --exec python. I've hit some long import chains that needed everything already up and running in order to have something similar working. To confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran GRASS GIS, accessed the interactive terminal, entered the Python shell within that environment, and executed the import statement without encountering any errors.

Copy link
Member

@echoix echoix Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, I see that this file is for generating docs (sphinx). Does someone know how to trigger it to check it? I know that the docs are built daily on an OSGeo server

footer_tmpl = string.Template(
r"""
{% block footer %}<hr class="header">
Expand Down
5 changes: 2 additions & 3 deletions gui/wxpython/gmodeler/frame.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file is most probably fine.

Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,11 @@
IsDark,
)
from gui_core.wrap import TextEntryDialog as wxTextEntryDialog

wxModelDone, EVT_MODEL_DONE = NewEvent()

from grass.script.utils import try_remove
from grass.script import core as grass

wxModelDone, EVT_MODEL_DONE = NewEvent()


class ModelFrame(wx.Frame):
def __init__(
Expand Down
3 changes: 1 addition & 2 deletions gui/wxpython/startup/locdownload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,11 @@
from grass.grassdb.checks import is_location_valid
from grass.script.setup import set_gui_path

set_gui_path()

from core.debug import Debug
from core.gthread import gThread
from gui_core.wrap import Button, StaticText

set_gui_path()
Comment on lines 35 to +37
Copy link
Member

Choose a reason for hiding this comment

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

Has there been a careful analysis of what is needed for gui_core.wrap import to be successful, and what does set_gui_path provide?


# TODO: labels (and descriptions) translatable?
LOCATIONS = [
Expand Down
33 changes: 17 additions & 16 deletions gui/wxpython/web_services/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,23 @@

import re
import os
import wx
import sys
import shutil
import grass.script as grass
import wx.lib.colourselect as csel

from copy import deepcopy
from core import globalvar

from xml.etree.ElementTree import ParseError

import wx

if globalvar.wxPythonPhoenix:
try:
import agw.flatnotebook as FN
except ImportError: # if it's not there locally, try the wxPython lib.
import wx.lib.agw.flatnotebook as FN
else:
import wx.lib.flatnotebook as FN
import wx.lib.colourselect as csel

from core.debug import Debug
from core.gcmd import GMessage
from core.gconsole import CmdThread, GStderr, EVT_CMD_DONE, EVT_CMD_OUTPUT

from web_services.cap_interface import (
WMSCapabilities,
WMTSCapabilities,
OnEarthCapabilities,
)

from gui_core.widgets import GNotebook
from gui_core.widgets import ManageSettingsWidget
from gui_core.wrap import (
Expand All @@ -59,7 +47,20 @@
TreeCtrl,
)

import grass.script as grass

def import_flatnotebook():
if globalvar.wxPythonPhoenix:
try:
import agw.flatnotebook as FN
except ImportError: # if it's not there locally, try the wxPython lib.
import wx.lib.agw.flatnotebook as FN
else:
import wx.lib.flatnotebook as FN
return FN


FN = import_flatnotebook()


rinwms_path = os.path.join(os.getenv("GISBASE"), "etc", "r.in.wms")
if rinwms_path not in sys.path:
Expand Down
7 changes: 4 additions & 3 deletions python/grass/docs/conf.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file I'm ok with.

Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,13 @@

import sys
import os
from datetime import date
import string
from datetime import date
from shutil import copy

# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
if not os.getenv("GISBASE"):
sys.exit("GISBASE not defined")
Copy link
Member

Choose a reason for hiding this comment

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

This change of moving the check under has the sys.path.insert calls won't have the expected outcome. Starting at line 24, that environment variable is used. Having the check after having finished using it isn't equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out. I've reconsidered the placement of the check and modified the code to ensure its effectiveness. I will push the changes to update the PR.

sys.path.insert(
0, os.path.abspath(os.path.join(os.environ["GISBASE"], "etc", "python", "grass"))
)
Expand Down Expand Up @@ -73,6 +71,9 @@
),
)

if not os.getenv("GISBASE"):
sys.exit("GISBASE not defined")

from grass.script import core

footer_tmpl = string.Template(
Expand Down
17 changes: 11 additions & 6 deletions python/grass/pygrass/raster/__init__.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file I'm ok with.

Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
from grass.script import fatal
from grass.exceptions import OpenError

import grass.lib.gis as libgis
import grass.lib.raster as libraster
import grass.lib.rowio as librowio

libgis.G_gisinit("")

#
# import pygrass modules
#
Expand All @@ -29,6 +23,17 @@
from grass.pygrass.raster.segment import Segment
from grass.pygrass.raster.rowio import RowIO


def import_grass_libs():
import grass.lib.gis as libgis
libgis.G_gisinit("")
import grass.lib.raster as libraster
import grass.lib.rowio as librowio
return libgis, libraster, librowio


libgis, libraster, librowio = import_grass_libs()
Copy link
Member

Choose a reason for hiding this comment

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

Using that pattern might complicate static analysis, as the imports aren't directly visible (it is a kind of lazy loading, but the call to that function is made during a parsing of the file for an import for example).

Does the lib raster and lib rowio import successfully and work correctly if libgis.G_gisinit("") isn't called strictly before, but underneath?

This pattern seems to be on multiple files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for highlighting the concern. I agree, and to address it, I've modified the code on all the changed files to keep the imports at the module level for direct visibility while maintaining the lazy loading pattern. I will update the PR with these changes


WARN_OVERWRITE = "Raster map <{0}> already exists and will be overwritten"

test_raster_name = "Raster_test_map"
Expand Down
21 changes: 12 additions & 9 deletions python/grass/pygrass/tests/benchmark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
"""

import optparse

# import numpy as np
import time
import collections
import copy
Expand All @@ -15,14 +13,19 @@
import os
from jinja2 import Template

sys.path.append(os.getcwd())
sys.path.append("%s/.." % (os.getcwd()))

import grass.lib.gis as libgis
import grass.lib.raster as libraster
import grass.script as core
import grass.pygrass
import ctypes
def import_grass_libs():
sys.path.append(os.getcwd())
sys.path.append("%s/.." % (os.getcwd()))
import grass.lib.gis as libgis
import grass.lib.raster as libraster
import grass.script as core
import grass.pygrass
import ctypes
return libgis, libraster, core, grass.pygrass, ctypes


libgis, libraster, core, pygrass, ctypes = import_grass_libs()


def test__RasterSegment_value_access__if():
Expand Down
17 changes: 11 additions & 6 deletions python/grass/pygrass/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,23 @@
import os
from sqlite3 import OperationalError

import grass.lib.gis as libgis

libgis.G_gisinit("")
import grass.lib.raster as libraster
from grass.lib.ctypes_preamble import String
from grass.script import core as grasscore
from grass.script import utils as grassutils

from grass.pygrass.errors import GrassError


def import_grass_libs():
import grass.lib.gis as libgis
libgis.G_gisinit("")
import grass.lib.raster as libraster
from grass.lib.ctypes_preamble import String
return libgis, libraster, String


libgis, libraster, String = import_grass_libs()

test_vector_name = "Utils_test_vector"

test_raster_name = "Utils_test_raster"


Expand Down
13 changes: 9 additions & 4 deletions python/grass/pygrass/vector/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
from os.path import join, exists
import grass.lib.gis as libgis

libgis.G_gisinit("")
import grass.lib.vector as libvect
import ctypes

#
Expand All @@ -19,6 +15,15 @@
from grass.pygrass.vector.basic import Bbox, Cats, Ilist


def import_grass_libs():
import grass.lib.gis as libgis
libgis.G_gisinit("")
import grass.lib.vector as libvect
return libgis, libvect


libgis, libvect = import_grass_libs()

_NUMOF = {
"areas": libvect.Vect_get_num_areas,
"dblinks": libvect.Vect_get_num_dblinks,
Expand Down
22 changes: 8 additions & 14 deletions scripts/r.in.wms/wms_drv.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This file I'm ok with.

Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,16 @@

import socket
import grass.script as grass
import numpy as Numeric
Copy link
Member

Choose a reason for hiding this comment

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

I know it was there before, but probably a new PR for this, but it is almost an universal convention across all Python code that numpy is imported as np, like import numpy as np. Why should we be different here?

Probably someone from this repo can jump in.

Copy link
Member

Choose a reason for hiding this comment

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

It was added a long time ago in 31d721a - perhaps the style of back then? Can @landam remember?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is just some very old practice carried over. Just change that.


from time import sleep
from math import pi, floor
from urllib.error import HTTPError
from http.client import HTTPException
from xml.etree.ElementTree import ParseError
from wms_base import GetEpsg, GetSRSParamVal, WMSBase
from wms_cap_parsers import WMTSCapabilitiesTree, OnEarthCapabilitiesTree
from srs import Srs

try:
from osgeo import gdal
Expand All @@ -30,22 +38,8 @@
)
)

import numpy as Numeric

Numeric.arrayrange = Numeric.arange

from math import pi, floor

from urllib.error import HTTPError
from http.client import HTTPException

from xml.etree.ElementTree import ParseError

from wms_base import GetEpsg, GetSRSParamVal, WMSBase

from wms_cap_parsers import WMTSCapabilitiesTree, OnEarthCapabilitiesTree
from srs import Srs


class WMSDrv(WMSBase):
def _download(self):
Expand Down
Loading