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

[Keyboard] add arrowmechanics/wings #23328

Merged
merged 3 commits into from
Mar 30, 2024
Merged

Conversation

philvec
Copy link
Contributor

@philvec philvec commented Mar 21, 2024


Description

  • implement all essential funcionality for wings kb
  • add default layout

this PR has been re-created after cleaning up the mess in the old invalid one (closed): #23227

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

* implement all base func for wings kb

* add readme.md

* improve layout definition

* apply guidelines froom pr checklist

* fix encoder indexing warning

* Apply suggestions from code review

Co-authored-by: Joel Challis <git@zvecr.com>

* enable full duplex uart, fix rgb matrix, change handedness method

* update default keymap

* fix typos in readme.md

* add split sync options for rgb, change default color and sound

* fix incorrect encoders' config

* reformat json

* hotfix encoder-index linter error

* Apply suggestions from code review

Co-authored-by: Joel Challis <git@zvecr.com>

* Update n.o. RGB LEDs

* Apply reviewers' suggestions

Co-authored-by: Drashna Jaelre <drashna@live.com>

---------

Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
@zvecr
Copy link
Member

zvecr commented Mar 21, 2024

This PR seems to have too many comments/issues vs. few changes that it actually introduces. Closing to create a clean version since it may take too much time to dig into things that are already fixed.

Exactly the opposite, and this should not have been done. Now you have created more work for reviewers as we need to cross-reference the old PR to make sure all comments are addressed.

@philvec
Copy link
Contributor Author

philvec commented Mar 21, 2024

Despite the facts, I really don't want to cause trouble.
BUT why exactly do You need to cross-reference it, can't we treat it as a different PR (what it actually IS) ? By the time I was mainly just applying the reviewers' suggestions, numerous other PRs were merged, some without having the checks passed, some created later than this one. Some side-comments of mine (like the one mentioning a possible issue with more important code) were not addressed at all though there were multiple reviewers' activity marks. Are You sure these are all good practices?
On the other hand, since I was just applying the suggestions, the job that needed to be done there as well as the mentioned cross-referencing here would require less keypresses (and thus less time for a kayboard-geek I guess? 😆 ) than writing Your comment. Literally reading outloud all the lines of code in this PR would take less time than You guys spent together writing these comments and seeing my responses. Sorry it went this way, but I really don't want that.
So here it is, a new PR, consider it a different story, do the review from scratch - it will come easier (and sooner!) anyway, thats how it seems.
Or otherwise, what is the real problem here?

Honestly, I understand this is Your time You guys commit, but I guess there is also responsibility. If its real issues and You are here and see them, please tell me what to improve and I will do so. I will do anything I can to speed this up, but I guess I shouldnt be saying 'please' and write other non-statement-like, not-directly-code-related stuff, because its not considered a good practice, right?
PLEASE guys, just do the review, I am torturing the refresh button here for two weeks already. 😢

@tzarc
Copy link
Member

tzarc commented Mar 22, 2024

BUT why exactly do You need to cross-reference it,

Because too many people close PRs with a whole bunch of requested changes that they don't feel like doing, reopening a new one with the hopes that we don't pull them up on the same requested changes.

We now need to refer to the old one, and for each and every requested change you're asking us to refer to TWO pull requests -- what did we ask for last time, and did you actually fix it this time.

Repeat for the next PR reviewer.

These are the sorts of supposedly "helpful" things that make things much, much worse for people reviewing.

We understand you feel super happy and excited about your PR, but at this stage there are 324 other PRs open, and from our perspective, yours isn't special. You'll have to wait for someone to get to it.

@philvec
Copy link
Contributor Author

philvec commented Mar 22, 2024

#23320

  • just another keyboard one from the list - less than 2 days, numerous commits and comments - 2 approving reviews already
    How does one make their PR "special" ?

@tzarc
Copy link
Member

tzarc commented Mar 22, 2024

There are no "special" PRs -- that's the entire point. Some of my own open PRs are still unreviewed by others since early January.
And besides, that PR has one approving review, not two. All the churn is from dunk2k, who is not a QMK maintainer.

To gain some perspective, please have a read of these links:

I'll also point out that debating the situation and getting us to take the time to describe why things are as they are takes time away from actual reviews. Food for thought.

@philvec
Copy link
Contributor Author

philvec commented Mar 22, 2024

Yeah, I get it, I get it... Really didn't want to demand anything from anyone, was just curious what else I can do. The last argument of Yours I mentioned myself above. I am a geek too for years, I know the OS vibe. I will stop commenting, just consider this perspective: if there's anything I can do, review/write any needed code myself, walk Your dog, do the chores - I'd really love to if only I could. If I can contribute anyhow in particular, if there's any task around QMK nobody wants to do - please tell me, maybe it so happens that I have the skills - I do Python for a living but I know C since I was 10 - I can spend a full day or two on that. filip@arrowmechanics.com
I'm not the one demanding, I'm the one excited, I hope that keeps You guys from getting burnt out 😉

keyboards/arrowmechanics/wings/info.json Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/info.json Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/readme.md Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/readme.md Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/readme.md Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/mcuconf.h Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/halconf.h Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/config.h Outdated Show resolved Hide resolved
keyboards/arrowmechanics/wings/info.json Outdated Show resolved Hide resolved
@philvec
Copy link
Contributor Author

philvec commented Mar 22, 2024

@nooges by the last commit I guess You may be the one who understands <3 also, thanks @dunk2k but I'll let sb check Your suggestions as I am not sure and don't want to cause more hussle

keyboards/arrowmechanics/wings/info.json Outdated Show resolved Hide resolved
…o GPL3, update readme - tested OK for build and withhardware
@philvec philvec requested a review from dunk2k March 22, 2024 19:04
@drashna drashna requested review from a team and removed request for dunk2k March 29, 2024 05:21
@waffle87 waffle87 merged commit 6dbc1b8 into qmk:master Mar 30, 2024
3 checks passed
@philvec
Copy link
Contributor Author

philvec commented Mar 31, 2024

THANK YOU AND HAPPY EASTER

mzyt pushed a commit to mzyt/qmk_firmware that referenced this pull request May 6, 2024
Co-authored-by: Arrow Mechanics <filip@arrowmechanics.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
girtsf pushed a commit to girtsf/qmk_firmware that referenced this pull request Jun 5, 2024
Co-authored-by: Arrow Mechanics <filip@arrowmechanics.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
whoisjordangarcia pushed a commit to whoisjordangarcia/qmk_firmware that referenced this pull request Jun 8, 2024
Co-authored-by: Arrow Mechanics <filip@arrowmechanics.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
Co-authored-by: Arrow Mechanics <filip@arrowmechanics.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants