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

Use pathlib everywhere we can #7872

Merged
merged 9 commits into from
Feb 17, 2020
Merged

Use pathlib everywhere we can #7872

merged 9 commits into from
Feb 17, 2020

Conversation

skullydazed
Copy link
Member

This PR switches to using pathlib for all path related code. While we take a performance hit in some places the overall gain in code readability is well worth it.

Types of Changes

  • Core
  • Enhancement/optimization

Checklist

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@skullydazed skullydazed added cli qmk cli command python labels Jan 12, 2020
@skullydazed skullydazed requested a review from a team January 12, 2020 06:25
Erovia
Erovia requested changes Jan 14, 2020
View reviewed changes
lib/python/qmk/path.py Outdated Show resolved Hide resolved
lib/python/qmk/cli/json/keymap.py Show resolved Hide resolved
lib/python/qmk/cli/json/keymap.py Show resolved Hide resolved
lib/python/qmk/tests/test_qmk_path.py Show resolved Hide resolved
lib/python/qmk/tests/test_qmk_path.py Outdated Show resolved Hide resolved
@Erovia Erovia mentioned this pull request Jan 14, 2020
Merged
8 tasks
@zvecr zvecr mentioned this pull request Jan 19, 2020
Merged
13 tasks
Erovia
Erovia reviewed Jan 24, 2020
View reviewed changes
lib/python/qmk/path.py Outdated

if os.path.exists(basepath):
return basepath
for i in range(5):
Copy link
Member

Choose a reason for hiding this comment

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

This 5 feels like a magic number here. Why 5?
This code is walking up the directory tree, right? I think a while keyboard_folder != Path('keyboards') would be more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the number of subfolders we support when building keyboards.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about while keyboard_folder != Path('keyboards') so we don't have to hardcode a number?

Erovia
Erovia requested changes Jan 24, 2020
View reviewed changes
lib/python/qmk/path.py Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team January 25, 2020 22:15
@drashna
Copy link
Member

drashna commented Jan 25, 2020

Travis is failing:

https://travis-ci.org/qmk/qmk_firmware/builds/641857334#L685

@skullydazed skullydazed force-pushed the pathlib_things branch 4 times, most recently from 1488479 to 2c3a876 Compare February 16, 2020 18:41
@skullydazed skullydazed requested a review from Erovia February 17, 2020 17:12
Erovia
Erovia approved these changes Feb 17, 2020
View reviewed changes
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

LGTM

@skullydazed skullydazed merged commit c669304 into master Feb 17, 2020
@skullydazed skullydazed deleted the pathlib_things branch February 17, 2020 19:42
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
@skullydazed @Erovia @HokieGeek
Use pathlib everywhere we can (qmk#7872)
eb91b6d 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
nesth pushed a commit to nesth/qmk_firmware that referenced this pull request Feb 21, 2020
Merge remote-tracking branch 'upstream/master'
2a8ece2 
* upstream/master: (330 commits)
  Add Danish keymap and sendstring LUT (qmk#8218)
  format code according to conventions [skip ci]
  uart.c fix from TMK (qmk#7628)
  S75 Encoder Fixes (qmk#7758)
  Add Turkish keymap aliases and sendstring LUT (qmk#7676)
  Add Arm Teensys to mcu_selection.mk (qmk#8026)
  [New keyboard]silverbullet44 (qmk#7950)
  Allow 30us matrix delay to be keyboard/user overridable  (qmk#8216)
  Merge /prime_l and /prime_l_v2 (qmk#8194)
  [Keymap] Keymap for XD75 with 7U spacebar EN-RU gamers (qmk#8184)
  Add VIA support for kbd8x mk2 (qmk#8168)
  Move Grave/Tilde and some lesser used keys on my ergo boards (qmk#8200)
  [Keyboard] KC60SE cleanup (qmk#8171)
  Made windows driver installation accept y as All to allow CI (qmk#8189)
  Change the image photo of /keyboards/reviung41/readme.md (qmk#8195)
  MxSS RGB Handler Touchup (qmk#8105)
  Centromere Configurator layout support and readme update (qmk#8190)
  dynamic keymap sanity check (qmk#8181)
  [keyboard] Austin (qmk#8176)
  Use pathlib everywhere we can (qmk#7872)
  ...
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
@skullydazed @Erovia @c0psrul3
Use pathlib everywhere we can (qmk#7872)
5541f5b 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Mar 26, 2020
@skullydazed @Erovia @drashna
Use pathlib everywhere we can (qmk#7872)
72b85f5 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@skullydazed @Erovia @kujawakk
Use pathlib everywhere we can (qmk#7872)
82af294 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Apr 22, 2020
@skullydazed @Erovia @drashna
Use pathlib everywhere we can (qmk#7872)
7e01ebc 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
@skullydazed @Erovia @fdidron
Use pathlib everywhere we can (qmk#7872)
74bcafa 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
jakeisnt pushed a commit to jakeisnt/qmk_firmware that referenced this pull request Aug 20, 2020
@skullydazed @Erovia @jakeisnt
Use pathlib everywhere we can (qmk#7872)
cd1ad72 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@skullydazed @Erovia
Use pathlib everywhere we can (qmk#7872)
2e11797 
* Use pathlib everywhere we can

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Update lib/python/qmk/path.py

Co-Authored-By: Erovia <[email protected]>

* Improvements based on @Erovia's feedback

* rework qmk compile and qmk flash to use pathlib

* style

* Remove the subcommand_name argument from find_keyboard_keymap()

Co-authored-by: Erovia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Erovia Erovia Erovia approved these changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
cli qmk cli command python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skullydazed @drashna @Erovia