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

Use pathlib #226

Merged
merged 36 commits into from
Oct 6, 2020
Merged

Use pathlib #226

merged 36 commits into from
Oct 6, 2020

Conversation

ju-sh
Copy link
Contributor

@ju-sh ju-sh commented Aug 29, 2020

Use pathlib

Description

  • Use pathlib for path operations.
  • Add some type annotations.

Related Issue

Mentioned at #224.

Checklist:

  • I have updated the documentation in the README.md file or my changes don't require an update.
  • I have added an entry in CHANGELOG.md.
  • I have added or adapted tests to cover my changes.

@ju-sh
Copy link
Contributor Author

ju-sh commented Aug 29, 2020

Hi.

Coverage has reduced but I thought it was better to see if the changes were okay before adding to the tests as I have probably broken something.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I had a quick look and the pathlib code looks good. However, reviewing this in more detail is hard, because most of the changes relate to adding type annotations. I haven't made up my mind about whether type annotations are useful for Vulture completely (given that it's primarily used as an app, not a library and has 100% test coverage), but I'm leaning towards adding them, at least where they make understanding the code easier. Therefore, I'd ask you to remove the type annotations, type imports and changes only related to mypy for now. Maybe you can store the diff related to types somewhere, for a separate PR. Depending on how separated your commits are, this may be easiest to to with a graphical diff tool like "meld".

tests/__init__.py Outdated Show resolved Hide resolved
@ju-sh
Copy link
Contributor Author

ju-sh commented Sep 8, 2020

Sorry for the delay. Got held up with something else in between.

I had ended up adding annotations to help me figure out the changes to make. I've removed them.

But, as an aside, I think it's good to have type annotations. They can really help a lot while making changes. Even when we don't do static type checking.

Let me know of further changes so we can go ahead and squash the commits.

tests/test_config.py Outdated Show resolved Hide resolved
Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

After the rebase, please go over the diff and check that only pathlib-related changes are present. For example, I still saw some type annotations, which make reviewing hard.

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
tests/test_encoding.py Show resolved Hide resolved
vulture/core.py Outdated Show resolved Hide resolved
Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

This looks much nicer :-) I think with my proposed changes, we can get rid of some pathlib->string conversions, which should make the code cleaner.

vulture/core.py Show resolved Hide resolved
vulture/core.py Outdated Show resolved Hide resolved
vulture/utils.py Outdated Show resolved Hide resolved
vulture/utils.py Show resolved Hide resolved
tests/test_script.py Outdated Show resolved Hide resolved
vulture/core.py Outdated Show resolved Hide resolved
vulture/utils.py Outdated Show resolved Hide resolved
vulture/utils.py Outdated Show resolved Hide resolved
tests/test_encoding.py Outdated Show resolved Hide resolved
vulture/core.py Outdated Show resolved Hide resolved
@jendrikseipp jendrikseipp merged commit d87d796 into jendrikseipp:master Oct 6, 2020
@jendrikseipp
Copy link
Owner

Thanks for your work on this!

The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Jan 19, 2021
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