Overhaul modifier handling system

[eltang]

See #272 for a detailed description of the changes.

[eltang]

So you're holding off on this one too?

[jackhumbert]

Yeap! This goes even deeper into the codebase. Plus it modifies @ezuk's stuff, so I'd like to let him take a look at things.

[ezuk]

Eric: since your PRs run in depth, they should come with automated tests.
Make a test documenting the original functionality, refactor, and show that
the test is still passing. That's the only way to overhaul things safely at
the level you're attempting.

On Sat, Apr 23, 2016, 07:27 Jack Humbert notifications@github.com wrote:

Yeap! This goes even deeper into the codebase. Plus it modifies @ezuk
https://github.com/ezuk's stuff, so I'd like to let him take a look at
things.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#277 (comment)

[eltang]

@ezuk I've never heard of this kind of test before. How exactly should I go about making them? What exactly is their purpose? I've been using this code in my keyboard for many months without issue.

[jackhumbert]

If you could also outline the differences between this and the current implementation (pros/cons of each), that'll help us decide if it's appropriate.

[eltang]

What's still not clear? I thought I explained it pretty well in the issue.

[ezuk]

The tldr for me is that I have to be confident you're not breaking other
people's work (keymaps), and right now I'm far from confident.

You open PRs and take them back, change things all over the place, and it
generally feels like you're really rushing it.

Hence my insistence on automated test coverage. And I'd like you to take
the time to research it and come up with an automated test framework you
like.

A text would show the syntax used in a keymap (e.g LCTL) and that it still
ends up executing the right parts of the code and doing the right thing.

On Sat, Apr 23, 2016, 14:46 Eric Tang notifications@github.com wrote:

What's still not clear? I thought I explained it pretty well in the issue.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#277 (comment)

[eltang]

Well, I'm only changing the other ones because @jackhumbert is not going to merge them later. This one hasn't changed much at all.

I've never done this kind of test before and I'm gonna have to learn a bit in order to do one. As I already mentioned, I've been using this in my keyboard for months without issue. Is that not enough?

Would it be okay if I described the old and new behaviors in detail like @jackhumbert recommended?

[ezuk]

The fact that a given change doesn't break your personal keymap is wonderful. However, that doesn't mean it won't break other people's work -- there are many ways to use QMK.

Thoroughly describing your work is key of course - that should come as part of the PR description (that's why it's there), and as comments in the code (though too many comments mean your code doesn't speak for itself, which is a problem).

That is not a replacement for having automated tests. I recognize this isn't something we've done so far, but that's also because most changes so far have been incremental and fairly conservative. I love the verve and energy with which you approach the project -- it just means we're leveling up. Part of this needs to be introducing those tests -- it goes with refactoring any legacy codebase, essential to making sure you're not breaking stuff.

Here are a few links that may help:

1. http://c.learncodethehardway.org/book/ex30.html
2. http://www.drdobbs.com/testing/unit-testing-in-c-tools-and-conventions/240156344
3. https://en.wikipedia.org/wiki/List_of_unit_testing_frameworks#C

[eltang]

@jackhumbert @ezuk This testing stuff is little overwhelming to me. Do you know if another community member might have the know-how to run them?

[ezuk]

@eltang I don't know of another community member who set out to change things in the scope you're looking to change. That's wonderfully ambitious -- but the responsibility for writing the tests lies with whoever changes the code. :)

Otherwise we find ourselves in the position of approaching someone and telling them, "Eric wants to change a bunch of things, can you please write tests to make sure his changes don't break what's working?" That's a tad awkward. :)

[eltang]

Well, #188 and #239 were pretty similar, and this PR is meant to finish what they started. Can't we handle this PR like #188 and fix things later in the unlikely event that someone's keymap breaks? I'll be responsible for doing it.

[ezuk]

The thing is, when something breaks we don't always know why. Bugs can manifest in surprising ways. It can be hard to track down an issue to your commit, and not everybody can use git bisect and git blame to pinpoint these things. So here's what I suggest:

1. Let's do a smoke test. That basically means automatically compiling all existing keymaps in the project and seeing that they still compile and nothing explodes. If they build, that's at least something.

2. Some basic unit tests at least for your changes. Just start. Find a framework you like and write something. I bet others would pitch in with feedback.

3. If you feel this is slowing you down, make a branch in this repo (not your own fork, so you're still contributing and participating in the main qmk repo). Call that branch eric and your changes will live there, available for anyone to use. Then you can proceed with 1 and 2 above whenever you like, and merge to master when it's truly ready.

How does that sound?

[ezuk]

Also, please see https://github.com/buserror/simavr - can help with testing.

[NoahAndrews]

make a branch in this repo (not your own fork, so you're still contributing and participating in the main qmk repo)

He won't have permission to do that. There's nothing wrong with making a branch in his own QMK fork and having discussion about it here. Once it's ready, he can make a pull request from that branch to this repo's master branch.

[ezuk]

@NoahAndrews - Eric specifically wanted to contribute to this repo, which is why I mentioned this. We can make the branch for him if he chooses to go this route :)

[eltang]

@ezuk The smoke test will be done using this script, correct? I tried to install the simulator, but I got a ton of errors. Any ideas? I got it installed, but I still have to figure out how to run it.

[NoahAndrews]

@ezuk Except he wouldn't be able to push to it without becoming a full collaborator.

[NoahAndrews]

@eltang That script only checks against the Ergodox EZ keymaps.

[eltang]

@NoahAndrews How do I test the other ones, if needed?

[NoahAndrews]

@eltang Perhaps write similar scripts for other keyboards.

[eltang]

@ezuk Also, what exactly should I be testing in my tests? That's not too clear to me, especially because this PR does not simply refactor the code.