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

feat: Resolve relative URIs in nitpick.styles.include #470

Merged
merged 2 commits into from
Mar 26, 2022
Merged

feat: Resolve relative URIs in nitpick.styles.include #470

merged 2 commits into from
Mar 26, 2022

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Mar 21, 2022 •
edited
Loading

Proposed changes

  • Refactored fetchers to operate on furl parsed objects
  • Relative URIs are resolved against the URI of the style file they are
    contained in.
  • During normalization, gh:https:// is mapped to github:https://, and
    pypackge:https:// is mapped to py:https:// to avoid loading styles more than
    once.

Fix: #464

Checklist

  • Read the contribution guidelines
  • Run make locally before pushing commits
  • Add tests for the relevant parts:
    • API
    • CLI
    • flake8 plugin (normal mode)
    • flake8 plugin (offline mode)
  • Write documentation when there's a new API or functionality

@mjpieters mjpieters force-pushed the relative_uris branch 2 times, most recently from 30b9cc8 to be18977 Compare March 21, 2022 19:25
@andreoliwa andreoliwa added this to In progress in Nitpick Roadmap Mar 21, 2022
@mjpieters mjpieters marked this pull request as draft March 21, 2022 20:28
@mjpieters
Copy link
Contributor Author

I’ll have to revisit the file url handling; tomorrow.

@mjpieters mjpieters marked this pull request as ready for review March 22, 2022 16:30
@mjpieters mjpieters force-pushed the relative_uris branch 6 times, most recently from d78db90 to 5b946b1 Compare March 22, 2022 17:39
@mjpieters
Copy link
Contributor Author

Gah, I need a better way of running Windows tests locally.

@andreoliwa
Copy link
Owner

Gah, I need a better way of running Windows tests locally.

If you find something, let me know. 😅

I don't have a Windows VM anymore, so last time I was using print() on GitHub Actions.
Trial and error. 🤷🏻‍♂️

@mjpieters
Copy link
Contributor Author

If you find something, let me know. 😅

I'm in the middle of a laptop switch, and then I'll try out https://github.com/StefanScherer/windows-docker-machine.

@mjpieters
Copy link
Contributor Author

I gave up on the vagrant / docker route and used Parallels instead. I have this down to a single failure now, should be able to get this completed tomorrow.

@mjpieters mjpieters force-pushed the relative_uris branch 2 times, most recently from 9cb7a93 to 075e23b Compare March 23, 2022 21:30
@mjpieters
Copy link
Contributor Author

Aaaand, we have a green build. This is ready for review now.

@coveralls
Copy link

coveralls commented Mar 23, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2044341989

  • 219 of 227 (96.48%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 96.434%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nitpick/style/fetchers/github.py 47 48 97.92%
src/nitpick/style/fetchers/pypackage.py 29 30 96.67%
src/nitpick/generic.py 6 12 50.0%
Files with Coverage Reduction New Missed Lines %
src/nitpick/style/core.py 1 83.33%
Totals Coverage Status
Change from base Build 2022260247: -0.4%
Covered Lines: 2009
Relevant Lines: 2065
💛 - Coveralls

@mjpieters
Copy link
Contributor Author

Side note: when refactoring I already was replacing @lru_cache() use on methods with simpler beg-for-forgiveness caches in an attribute, just because you really don't need the overhead of @lru_cache() here.

And then flake8-bugbear added a B019 error message because the cache holds a reference to self in the cache key.

Sooner or later you'll have to deal with the remaining 11 uses of @lru_cache() on methods!

@mjpieters
Copy link
Contributor Author

mjpieters commented Mar 24, 2022 •
edited
Loading

Further changes committed addressing:

  • One of the @xfail tests expected to fail on Windows. It now passes.
  • The issues that fixing that test uncovered.
  • A partial fix for the other failures in that Fuss() objects where not being sorted correctly. They were effectively never sorted as the __lt__ method would always return False except in very, very narrow and rare circumstances.

I'll plug some more at the remaining xfails tomorrow, I’m pretty sure one of these is only failing locally because I haven’t quite configured symlinks correctly yet and the other is probably more proper TOML string escaping (can’t just expect backslashes in paths to work).

@mjpieters
Copy link
Contributor Author

Okay, this is now definitely, finally, complete. All Windows XFAILs are now gone.

@andreoliwa: please review and let me know how this is sitting with you. It's a pretty fundamental feature I'd need for our use of nitpick to be sustainable in the long run!

@mjpieters
feat: Resolve relative URIs in nitpick.styles.include
a6789c9 
- Refactored fetchers to operate on furl parsed objects
- Relative URIs are resolved against the URI of the style file they are
  contained in.
- During normalization, `gh:https://` is mapped to `github:https://`, and
  `pypackge:https://` is mapped to `py:https://` to avoid loading styles more than
  once.
@andreoliwa
Copy link
Owner

I'll review this today after work or latest this weekend.
Thank you again for this improvement!

@mjpieters mjpieters mentioned this pull request Mar 25, 2022
Merged
8 tasks
andreoliwa
andreoliwa approved these changes Mar 26, 2022
View reviewed changes
Copy link
Owner

@andreoliwa andreoliwa left a comment

Choose a reason for hiding this comment

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

Amazing. You added a feature, improved the code readability, cleaned up, fixed all Windows errors. I'll merge this right away. 🙇🏻‍♂️

docs/nitpick_section.rst Show resolved Hide resolved
src/nitpick/project.py Show resolved Hide resolved
src/nitpick/project.py Show resolved Hide resolved
src/nitpick/style/fetchers/__init__.py Show resolved Hide resolved
src/nitpick/style/fetchers/github.py Show resolved Hide resolved
src/nitpick/violations.py Show resolved Hide resolved
tests/test_style.py Outdated Show resolved Hide resolved
@andreoliwa andreoliwa merged commit ec934dc into andreoliwa:develop Mar 26, 2022
Nitpick Roadmap automation moved this from In progress to Done Mar 26, 2022
github-actions bot pushed a commit that referenced this pull request Mar 27, 2022
@semantic-release-bot
chore(release): 0.32.0 [skip ci]
66fa857 
# [0.32.0](v0.31.0...v0.32.0) (2022-03-27)

### Bug Fixes

* **deps:** update dependency pytest-socket to a commit hash ([#440](#440)) ([61ac278](61ac278))
* GitHub URL should preserve query args ([#453](#453)) ([a2b97b1](a2b97b1))
* use built-in preset as default style ([#450](#450)) ([68fa2ce](68fa2ce))

### Features

* add --version cli switch (thanks to [@mjpieters](https://github.com/mjpieters)) ([#468](#468)) ([6a85f79](6a85f79))
* resolve relative URIs in nitpick.styles.include ([#470](#470)) ([ec934dc](ec934dc))
* set initial style url(s) with nitpick init ([#473](#473)) ([0100f2b](0100f2b))
* switch to requests-cache for style caching ([#467](#467)) ([c586d7f](c586d7f))
@github-actions
Copy link

🎉 This PR is included in version 0.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Feature/fix is released label Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreoliwa andreoliwa andreoliwa approved these changes

Assignees
No one assigned
Labels
released Feature/fix is released
Projects
Nitpick Roadmap
Archived in project
Nitpick Roadmap
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relative paths in [nitpick.styles].includes remote style should be resolved against the remote URL
3 participants
@mjpieters @andreoliwa @coveralls