You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add test in scripts repo, to avoid manual documentation of keybinds #988
This is automatically generated (see #972), which guarantees it will remain up-to-date. If a keybinding is suggested, it should simply be added to dfhack.init-example instead of the script documentation.
A test should be added to enforce this, probably by pattern-matching "keybinding add" or "bound to" and so on within the docs section.
Yes, but due to the obvious false-positive issue it should probably be a very restricted blacklist. Part of the purpose is to have something in the scripts repo that tells people docs don't need to include keybindings, so just banning keybinding add commands should do - there's no other reason to use them I can think of, and if something comes up the test can always be modified later.
As a regexp "keybinding add (Ctrl-|)(Shift-|)(Alt-|)[A-Z](@.? | )" within the script documentation tokens should work. I think, I'm not great with regex :/
Ctrl/Shift/Alt don't have to be in that order, so something like keybinding add (Ctrl-|Shift-|Alt-)*\w+(@.+)? should work. (Numbers, F1-F12, and Enter are supported too.)
Anyway, I feel like having a person verifying this would be less error-prone than automatically failing a test. Maybe a warning would work, though (if anyone reads it :) ).
Regarding #972 - that would add keybindings to the script docs as soon as they exist in dfhack.init-example, right? Just want to make sure it wouldn't require people to add anything to scripts they submit here, since that could cause issues if the corresponding dfhack.init-example changes haven't been merged (or thought of) yet.
Coming back to DFHack after a period of hibernation...
The principle behind this is that people don't read warnings unless the build fails, and we can probably make this happen. We can tolerate false-negatives (hopefully manual review will catch them), so long as the false-positive rate is low.
Yes, the idea of #972 is that scripts have nothing to do with it, and the note gets added automatically based on the contents of dfhack.init-example. Only plugins need to use the directive directly. In fact, our test should check that scripts aren't using it! (which would break when things change)
There were a couple unimplemented keybindings that I just fixed, but I think we can handle this manually in the future by deciding whether to add bindings to the default init file when PRs are made.
Activity
lethosor commentedon Aug 31, 2016
So you're effectively proposing blacklisting those phrases from any script docs?
dscorbett commentedon Aug 31, 2016
There’s bound to be false positives.
PeridexisErrant commentedon Sep 2, 2016
Yes, but due to the obvious false-positive issue it should probably be a very restricted blacklist. Part of the purpose is to have something in the scripts repo that tells people docs don't need to include keybindings, so just banning
keybinding addcommands should do - there's no other reason to use them I can think of, and if something comes up the test can always be modified later.As a regexp
"keybinding add (Ctrl-|)(Shift-|)(Alt-|)[A-Z](@.? | )"within the script documentation tokens should work. I think, I'm not great with regex :/lethosor commentedon Sep 2, 2016
Ctrl/Shift/Alt don't have to be in that order, so something like
keybinding add (Ctrl-|Shift-|Alt-)*\w+(@.+)?should work. (Numbers, F1-F12, and Enter are supported too.)Anyway, I feel like having a person verifying this would be less error-prone than automatically failing a test. Maybe a warning would work, though (if anyone reads it :) ).
Regarding #972 - that would add keybindings to the script docs as soon as they exist in dfhack.init-example, right? Just want to make sure it wouldn't require people to add anything to scripts they submit here, since that could cause issues if the corresponding dfhack.init-example changes haven't been merged (or thought of) yet.
PeridexisErrant commentedon Oct 20, 2016
Coming back to DFHack after a period of hibernation...
The principle behind this is that people don't read warnings unless the build fails, and we can probably make this happen. We can tolerate false-negatives (hopefully manual review will catch them), so long as the false-positive rate is low.
Yes, the idea of #972 is that scripts have nothing to do with it, and the note gets added automatically based on the contents of
dfhack.init-example. Only plugins need to use the directive directly. In fact, our test should check that scripts aren't using it! (which would break when things change)Add some suggested keybindings from docs to dfhack.init-example
lethosor commentedon May 16, 2018
There were a couple unimplemented keybindings that I just fixed, but I think we can handle this manually in the future by deciding whether to add bindings to the default init file when PRs are made.
Merges upstream. (git merge --squash upstream/master)