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

Fixes for failures during 'make lets_split' #4489

Closed
wants to merge 2 commits into from

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Nov 26, 2018

Before i make any changes to the lets_split board, i wanted to make sure the whole folder was building. The failures are as follows

@drashna
Copy link
Member

drashna commented Nov 27, 2018

Honestly, we're generally not a fan of editing other people's keymaps. It's impolite usually.

Also, for Travis related issues, we will ignore a keymap that causes issues, as long as the default keymap compiles fine.

Flagging somebody about the issue is okay, though.

@zvecr
Copy link
Member Author

zvecr commented Nov 27, 2018

From my background, failing code is failing code.

The reason i was making this change was I planned to swap the lets split over to using the shared split implementation. (already testing it at https://github.com/zvecr/qmk_firmware/tree/feature/ortho_4x12-keymap). However broken code makes these changes extremely difficult. You can no longer rely on the state of the code base being good, and investigate anything that is broken by your changes.

@drashna
Copy link
Member

drashna commented Nov 27, 2018

From my background, failing code is failing code.
Well, I don't know your background, or the projects that you've worked in. But I'm pretty sure that QMK is different from them, in a number of ways.

It's not just keyboard firmware, but a community as well. Jack, Skully and others have worked hard to make it that way, and make it welcoming for everyone.

So, it's not just that you're fixing failing code here, but you're also messing with somebody else's keyboard. So this isn't just about fixing layouts, it's also about respecting other people's property too.

Additionally, changing the keymaps could make it more difficult for that person to update their code (either locally for them, or in the qmk repo).

However broken code makes these changes extremely difficult.

How so? If the default keymap compiles, then it's fine. If the other keymaps don't, then as long as it's not directly related to the changes that were made, then they can be ignored.

Travis CI makes the testing process very easy, as well. If you haven't already, I would recommend setting it up (https://travis-ci.org).

Again, other than compiling errors for those keymaps, I don't see how they would cause issues with converting the let's split keyboard over to the split common code feature.

My personal policy has been to ignore issues with keymaps, if the default keymap compiles. And then tag people in a PR when Travis CI fails due to issues with their keymaps. That way, they know there is an issue and they can see where the issue is, so that they can fix the issue with their keymap, rather than having somebody else mess with it.

@zvecr
Copy link
Member Author

zvecr commented Nov 27, 2018

I mean no disrespect with my previous comment, and i understand about the case for other peoples keyboards.

However techical debt is a term that plagues all software projects. The state of the repo has changed over time, use of #pragma once, deprecated tmk macros, makefile changes, ect. A new contributor has to battle against these unwritten rules when they copy-paste something as their basis for change.

Surely we should spend effort to crush technical debt, and make the code base reflect the current standards?

Welcoming everyone goes both ways, so for now i think it better to close this PR and move on.

@zvecr zvecr closed this Nov 27, 2018
@drashna
Copy link
Member

drashna commented Nov 27, 2018

Well, if it helps, we're discussing this exact issue internally right now. So closing this may be a bit premature.

And more specifically, we need to figure out an official policy for how to handle keymaps in this sort of case. That way, everything is much clearer going forward.

@noroadsleft
Copy link
Member

Since it's relevant here:

I was actually thinking about doing a PR that only removes the

ifndef QUANTUM_DIR
	include ../../../../Makefile
endif

blocks, but does so from the whole repo.

I kind of agree with @zvecr's point about technical debt. Broken keymaps bother me. But I understand your point, @drashna, about respecting other people's property. Lately I've been trying to make as few changes as possible to other people's keymaps, and only if it's relevant to the goal of whatever my PR is.

One exception I have to that guideline is keymaps that are unmaintained, because what I've noticed is a lot of copy-paste contributions in keymaps, where a prospective QMK user finds a keymap they like, copies and edits it, and then submits a PR and gets hit with a bunch of review comments because the keymap they copied hadn't been updated in a year. I think that raises the "barrier to entry" a bit and is something I'd much prefer to avoid.

In the instances of unmaintained keymaps, I try to modernize them to current QMK standards as best as I am capable. Some things like Tap Dance I mostly leave alone, as I have no experience with some QMK features.

@zvecr zvecr deleted the feature/lets_split-fix-build branch April 27, 2020 23:59
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

4 participants