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

New vendor keymap for ryanbaekr rb87 #24035

Closed
wants to merge 3 commits into from

Conversation

ryanbaekr
Copy link
Contributor

Description

Added a new keymap for ryanbaekr/rb87 that will ship with the board for some orders

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).

@github-actions github-actions bot added the keymap label Jul 2, 2024
@tzarc
Copy link
Member

tzarc commented Jul 2, 2024

Dupe of #23713. Author perhaps thinks closing old and reopening new was a good idea. It was not.

Reviewers now need to compare discussion context between two PRs, which makes things even more problematic.

Given the dubious nature of whether or not this is a user keymap along with https://docs.qmk.fm/ChangeLog/20240526#migration-of-via-keymaps-to-via-team-control I’m closing this as it’s of time-limited usefulness.

@tzarc tzarc closed this Jul 2, 2024
@ryanbaekr
Copy link
Contributor Author

How is it dubious when I am the creator of the keyboard and the firmware and am the vendor?

@tzarc
Copy link
Member

tzarc commented Jul 2, 2024

Very well; if you’re adamant it’s not a user keymap then it’s not a user keymap.

Next issue: how would you like us to address the blatant disregard of the standard PR process and the seemingly underhanded method you’ve decided to try to fly under the radar to get your code merged?

@ryanbaekr
Copy link
Contributor Author

I have found it very difficult to communicate with the maintainers this go around. I originally opened #23680 which was closed under the assumption that this was not a vendor keymap, I left a comment but could not get a reply. I have no other way to get someone to take a look again if comments aren't checked so I opened #23713. After getting through the vendor keymap debate there, you and I had a quick back and forth about additional changes, which I then made. After that I waited, and given the replies had been so quick before, I pinged as I figured it had slipped through the cracks (guess we've both made bad assumptions at this point). You then served me the classic don't bug the maintainers we do it for free explanation, which I get... I've been on the other side of this too. So I waited 5 weeks and again figured if no one was looking at it I could open another PR as that had been successful the first time.

As we both know this is a bad solution because discussion is now split/lost, but given the track record of older PRs going stale and me now being explicitly told not to ping I'm not really sure what other option I had.

I guess I could've waited 10 more days for the bot to mark it as stale, but to me the PR was clearly stale already and I didn't see the need to wait for the bot.

I have gone around the standard PR process because I have felt like I had no other options. It is not at all my intention to try to be "underhanded" or "fly under the rader" I would much prefer if there was just transparency both ways.

@ryanbaekr
Copy link
Contributor Author

ryanbaekr commented Jul 2, 2024

I genuinely would like you to tell me what I should have done given the context. Opening an issue doesn't feel appropriate, opening another PR was clearly not appropriate, and I had been told to stop commenting in the discussion.

@tzarc
Copy link
Member

tzarc commented Jul 2, 2024

I apologise that you've had a suboptimal experience here, I really do. But you'd have to wait just like everyone else. I have my own PRs raised before your previous one and they haven't received reviews at all. Nobody is given preference.

I'll ask that you try to understand things from our perspective though:

  • we get hundreds of PRs per month, so yes, there's the potential for things to slip through the cracks
  • the description of the original PR is intentionally antagonistic ("IF THIS IS NOT CONSIDERED A VENDOR KEYMAP, PLEASE EXPLAIN WHY"), which instantly means some of the team won't even bother to touch the PR in the first place
  • you've worded your responses on the previous PR in such a way that to outside observers it seems it's a personal keymap you're trying to merge, which under normal circumstances would mean it's not a candidate for merge
  • you didn't say anything about shipping the keymap on a board until this PR
  • given the previously mentioned https://docs.qmk.fm/ChangeLog/20240526#migration-of-via-keymaps-to-via-team-control, VIA-enabled keymaps are scheduled to be deleted from this repo in 6 or 7 weeks anyway

I hope you can understand why this PR has had low priority within the team.

I'll reopen the previous PR seeing as it has far more of the context -- reviews will be up to the rest of the team to do as I have zero appetite to engage in this any further.

@ryanbaekr
Copy link
Contributor Author

The reason I wrote "IF THIS IS NOT CONSIDERED A VENDOR KEYMAP, PLEASE EXPLAIN WHY" in #23713 is because #23680 was immediately closed for being a user keymap without any verification that it actually was. I was worried that #23713 would just be immediately closed again if I didn't ask for reasoning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants