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

init: Use pathlib Path.read_text for readfile in the main executable #4234

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Aug 27, 2024

I explicitly don't want readfile and writefile to be changed in the same PR.
I don't see any coverage (pytest from here, nor gunittest from my fork (too slow to be used allways)), that runs into the init script. So the approach is to leave it for some time before changing.

The readfile function is only used inside that file, 10 references, in functions like create_gisrc(), set_mapset(), load_env(), csh_startup(), sh_like_startup(), classic_parser().
image

If it were to have an impact, we should see it easily.

I seriously don't think there are any difference, as Pathlib's read_text() function is really simple to understand, so I'm including it here for reference when reviewing my similar PRs:

    def read_text(self, encoding=None, errors=None, newline=None):
        """
        Open the file in text mode, read it, and close the file.
        """
        # Call io.text_encoding() here to ensure any warning is raised at an
        # appropriate stack level.
        encoding = io.text_encoding(encoding)
        return PathBase.read_text(self, encoding, errors, newline)

https://github.com/python/cpython/blob/fe85a8291d9aa11c9ce9e207c39ea0a0c35f9625/Lib/pathlib/_local.py#L591-L598

calling

    def read_text(self, encoding=None, errors=None, newline=None):
        """
        Open the file in text mode, read it, and close the file.
        """
        with self.open(mode='r', encoding=encoding, errors=errors, newline=newline) as f:
            return f.read()

https://github.com/python/cpython/blob/fe85a8291d9aa11c9ce9e207c39ea0a0c35f9625/Lib/pathlib/_abc.py#L736-L741

and write_text() (not used here):

    def write_text(self, data, encoding=None, errors=None, newline=None):
        """
        Open the file in text mode, write to it, and close the file.
        """
        # Call io.text_encoding() here to ensure any warning is raised at an
        # appropriate stack level.
        encoding = io.text_encoding(encoding)
        return PathBase.write_text(self, data, encoding, errors, newline)

https://github.com/python/cpython/blob/fe85a8291d9aa11c9ce9e207c39ea0a0c35f9625/Lib/pathlib/_local.py#L600-L607

calling

    def write_text(self, data, encoding=None, errors=None, newline=None):
        """
        Open the file in text mode, write to it, and close the file.
        """
        if not isinstance(data, str):
            raise TypeError('data must be str, not %s' %
                            data.__class__.__name__)
        with self.open(mode='w', encoding=encoding, errors=errors, newline=newline) as f:
            return f.write(data)

https://github.com/python/cpython/blob/fe85a8291d9aa11c9ce9e207c39ea0a0c35f9625/Lib/pathlib/_abc.py#L752-L760

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

I agree, if this is broken we will know quickly.

@wenzeslaus wenzeslaus changed the title lib/init/grass: Use pathlib Path.read_text for readfile init: Use pathlib Path.read_text for readfile in main executable Aug 27, 2024
@echoix echoix changed the title init: Use pathlib Path.read_text for readfile in main executable init: Use pathlib Path.read_text for readfile in the main executable Aug 27, 2024
@echoix echoix merged commit e0a0288 into OSGeo:main Aug 27, 2024
28 checks passed
@echoix echoix deleted the read-whole-file-readfile-init branch August 27, 2024 03:36
@neteler neteler added this to the 8.5.0 milestone Aug 27, 2024
@echoix
Copy link
Member Author

echoix commented Aug 28, 2024

@veroandreo I was wondering if you were using this week a build from main since this was merged. I was waiting for the end of the week before merging #4235, to see if anything would pop up.

@veroandreo
Copy link
Contributor

@veroandreo I was wondering if you were using this week a build from main since this was merged. I was waiting for the end of the week before merging #4235, to see if anything would pop up.

I am using main indeed, and I just recompiled this morning. What should I test?

@echoix
Copy link
Member Author

echoix commented Aug 28, 2024

Just opening and starting grass. It touched reading bash profiles. If it was broken you should've noticed it already.

@veroandreo
Copy link
Contributor

Just opening and starting grass. It touched reading bash profiles. If it was broken you should've noticed it already.

no, no issues

@echoix
Copy link
Member Author

echoix commented Aug 28, 2024

Just opening and starting grass. It touched reading bash profiles. If it was broken you should've noticed it already.

no, no issues

Great! We might as well proceed earlier with #4235, it only needs a new approval after fixing the intended merge conflict when this PR was merged cleared the approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants