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

grass.pygrass: VisibleMapset: fix reading search path #2584

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

petrasovaa
Copy link
Contributor

VisibleMapset class was not reading search path properly, instead it was always just writing PERMANENT only into the path.

Copy link
Member

@tmszi tmszi left a comment

Choose a reason for hiding this comment

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

Tested and work as expected.

@tmszi
Copy link
Member

tmszi commented Sep 19, 2022

It requires modifying the GridModule class tests.

@tmszi
Copy link
Member

tmszi commented Sep 20, 2022

Complex test with 3c8d6b8:

  1. SEARCH_PATH file doesn't exists (passed):
GRASS nc_spm_08_grass7/landsat:~ > file grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH: cannot open `grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH' (No such file or directory)

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat PERMANENT

GRASS nc_spm_08_grass7/landsat:~ > cat /tmp/visible_mapset_test.py 
#!/usr/bin/env python3

import sys

from grass.pygrass.gis import VisibleMapset

def main():
    vm = VisibleMapset(mapset=sys.argv[1])
    print(vm.read())

if __name__ == '__main__':
    main()

GRASS nc_spm_08_grass7/landsat:~ > python /tmp/visible_mapset_test.py $(g.gisenv get="MAPSET")
['landsat', 'PERMANENT']
  1. SEARCH_PATH file is empty (passed):
GRASS nc_spm_08_grass7/landsat:~ > file grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH: empty

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat

GRASS nc_spm_08_grass7/landsat:~ > python /tmp/visible_mapset_test.py $(g.gisenv get="MAPSET")
['landsat']
  1. SEARCH_PATH file exist (passed, and a new bug appear with g.mapsets module):
GRASS nc_spm_08_grass7/landsat:~ > file grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH: ASCII text

GRASS nc_spm_08_grass7/landsat:~ > cat grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat
PERMANENT
climate_2000_2012

GRASS nc_spm_08_grass7/landsat:~ > python /tmp/visible_mapset_test.py $(g.gisenv get="MAPSET")
['landsat', 'PERMANENT', 'climate_2000_2012', '']

It seems that with this last test example (Python code output) we have an extra unnecessary empty '' MAPSET name due the g.mapsets module writing an extra \n to the SEARCH_PATH file. PR #2586.

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$
$

self._write(lns)
return lns
try:
with open(self.spath, "rb") as f:
Copy link
Member

@tmszi tmszi Sep 20, 2022

Choose a reason for hiding this comment

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

open file func mode param arg should be "r" only according output bellow and we don't need decode readed lines:

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets operation=set mapset=landsat

GRASS nc_spm_08_grass7/landsat:~ > file grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH: ASCII text

Same for _write() method, open file func with mode param arg should be w only.

    def _write(self, mapsets):
        """Write to SEARCH_PATH file the changes in the search path

        :param list mapsets: a list of mapset's names
        """
        with open(self.spath, "w") as f:
            f.write("\n".join([m for m in mapsets if m in self.location.mapsets()]))

extend() method without decode func

    def extend(self, mapsets):
        """Add more mapsets to the search path

        :param list mapsets: a list of mapset's names
        """
        final = self.read()
        final.extend(
            [
                m
                for m in mapsets
                if m in self.location.mapsets() and m not in final
            ]
        )
        self._write(final)

f.write(b"\n".join([encode(m) for m in mapsets if m in ms]))
with open(self.spath, "w") as f:
ms = self.location.mapsets()
f.write("\n".join([m for m in mapsets if m in ms]))
Copy link
Member

Choose a reason for hiding this comment

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

Small note (it is not bug):

g.mapsets module write \n on the EOF for every line in the SEARCH_PATH file:

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets operation=set mapset=landsat

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets operation=add mapset=PERMANENT

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$

VisibleMapset class instance _write method do not write \n on the EOF on the last line in the SEARCH_PATH file:

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -l
Available mapsets:
PERMANENT climate_2000_2012 landsat tomas

GRASS nc_spm_08_grass7/landsat:~ > cat /tmp/test.py
#!/usr/bin/env python3

import sys

from grass.pygrass.gis import VisibleMapset

def main():
    vm = VisibleMapset(mapset=sys.argv[1])
    vm.extend(["climate_2000_2012", "tomas"])
    print(vm.read())

if __name__ == '__main__':
    main()

GRASS nc_spm_08_grass7/landsat:~ > python /tmp/test.py $(g.gisenv get="MAPSET")
['landsat', 'PERMANENT', 'climate_2000_2012', 'tomas']

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$
tomasGRASS nc_spm_08_grass7/landsat:~ >

tomasGRASS nc_spm_08_grass7/landsat:~ > this line should be (missing $ which is \n)

tomas$
GRASS nc_spm_08_grass7/landsat:~ >

Fixed with (line 459):

eof = "\n"
f.write(eof.join([m for m in mapsets if m in ms]) + eof)

and (line 469):

f.write("%s\n" % mapset)

@petrasovaa
Copy link
Contributor Author

No I got confused, didn't you @tmszi change the behavior of g.mapsets in #2586 to avoid writing the newline at the end? Should I revert the last commit in this PR?

@tmszi
Copy link
Member

tmszi commented Sep 23, 2022

No I got confused, didn't you @tmszi change the behavior of g.mapsets in #2586 to avoid writing the newline at the end? Should I revert the last commit in this PR?

With PR #2586 I solve problem with additional empty line ($ == \n) when g.mapsets module write to the SEARCH_PATH file.

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$
$

should be

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$

VisibleMapset class instance _write() method don't write \n on the last line ($ == \n) MAPSET -> MAPSET\n and your last commit a3e05df solve it and is needed:

Example when _write() method write to SEARCH_PATH file:

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$
tomasGRASS nc_spm_08_grass7/landsat:~ >

should be

GRASS nc_spm_08_grass7/landsat:~ > cat -e grassdata/nc_spm_08_grass7/landsat/SEARCH_PATH 
landsat$
PERMANENT$
climate_2000_2012$
tomas$
GRASS nc_spm_08_grass7/landsat:~ >

this line tomasGRASS nc_spm_08_grass7/landsat:~ > should be (missing \n on the EOF tomas to tomas$)

tomas$
GRASS nc_spm_08_grass7/landsat:~ >

@tmszi
Copy link
Member

tmszi commented Oct 4, 2022

Could we move forward with this PR, please?

@petrasovaa petrasovaa merged commit a94a3af into OSGeo:main Oct 6, 2022
@petrasovaa petrasovaa deleted the fix-pygrass-visible-mapset branch October 6, 2022 13:11
petrasovaa added a commit that referenced this pull request Oct 6, 2022
* handle missing search path file, do not write since this is only reading
* use text mode for writing/reading
* write newline after each mapset
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* handle missing search path file, do not write since this is only reading
* use text mode for writing/reading
* write newline after each mapset
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* handle missing search path file, do not write since this is only reading
* use text mode for writing/reading
* write newline after each mapset
marisn pushed a commit to marisn/grass that referenced this pull request Jun 2, 2023
* handle missing search path file, do not write since this is only reading
* use text mode for writing/reading
* write newline after each mapset
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* handle missing search path file, do not write since this is only reading
* use text mode for writing/reading
* write newline after each mapset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants