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

Prefer pathlib for read/write operations #591

Merged
merged 7 commits into from
Apr 29, 2024

Conversation

jaraco
Copy link
Contributor

@jaraco jaraco commented Apr 28, 2024

  • Rely on pathlib when reading and writing simple text.
  • Universal newlines is the default, so remove the comment.

Description

While working on #577, I noticed it might be nice to leverage pathlib for reading and writing files. This approach avoids adding nesting and with blocks by utilizing read_text and write_text.

This PR depends on #577 and should be rebased after merging that one.

Checklist

  • Make sure changes are covered by existing or new tests.
  • For at least one Python version, make sure local test run is green.
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes.
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • Ensure docs/tutorial.rst is still up-to-date.
  • If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
  • If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

@jaraco jaraco force-pushed the debt/pathlib-read branch 2 times, most recently from b6258b0 to f013fba Compare April 28, 2024 14:42
@jaraco
Copy link
Contributor Author

jaraco commented Apr 28, 2024

Oh, this change fails on Python 3.9 and earlier due to the newline parameter only being added in that edition.

If this proposed transition is acceptable, I'd be tempted to wrap the offending call in a compatibility wrapper (something that would signal its removal after Python 3.9 support is dropped), but I'm not sure it's worth the effort.

@jaraco jaraco force-pushed the debt/pathlib-read branch 2 times, most recently from 3acbaf3 to e5c6df7 Compare April 28, 2024 15:03
@jaraco
Copy link
Contributor Author

jaraco commented Apr 28, 2024

The coverage failure doesn't make any sense:

Name                       Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------
src/towncrier/_writer.py      25      0     10      1    97%   20->exit
----------------------------------------------------------------------
TOTAL                        677      0    321      1    99%

Does 20->exit mean that the coverage check wants to see an exception occur in the context in order to check both __exit__(None, None, None) and __exit__(type, val, ctx)? That's absurd and didn't need to be checked before. Or is it a bug in coverage, where it's failing to detect that the context is exited at all (which it must for the function to return)?

@jaraco jaraco force-pushed the debt/pathlib-read branch from 01dc868 to cb8076b Compare April 28, 2024 18:25
@jaraco jaraco marked this pull request as ready for review April 28, 2024 18:28
@jaraco jaraco requested a review from a team as a code owner April 28, 2024 18:28
src/towncrier/_writer.py Outdated Show resolved Hide resolved
@jaraco jaraco force-pushed the debt/pathlib-read branch from cb8076b to 163c42d Compare April 28, 2024 18:48
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks. Happy to use pathlib.

I left a few minor comments.


I don't line universal newlines.

On my projects, we always use \n for the source code.

You don't want a source file created on Windows to conflict with Linux.


So I think towncrier should just auto-detect the newlines used in the "filename" configuration and use that for "create" and other cases.

@adiroiban
Copy link
Member

Thanks. If you are happy with this PR, feel free to merge.

@jaraco
Copy link
Contributor Author

jaraco commented Apr 28, 2024

Thanks. If you are happy with this PR, feel free to merge.

I would merge, but something's wrong with credentials. I'm a member of towncrier-contributors, yet I have no option to merge. I've signed out and back in but yet can't merge. It says I need write access to merge.

image

@adiroiban adiroiban merged commit db10a4d into twisted:trunk Apr 29, 2024
15 checks passed
@adiroiban
Copy link
Member

Sorry. My bad. I set the wrong permissiosn for the team. I have merged it, but in the future you should be able to merge. Thanks again!

@jaraco jaraco deleted the debt/pathlib-read branch April 29, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants