Implement remaining tests for $if directive
#584
Conversation
| @@ -1,3 +1,4 @@ | |||
| module Reline | |||
| VERSION = '0.3.8' | |||
| COMPATIBLE_READLINE_VERSION = '8.2' | |||
There was a problem hiding this comment.
This is needed for the version test. I put the current version of Readline here, but I can't really say whether Reline is truly compatible with this version. Additionally, this will have to be kept up-to-date as necessary in order to properly support the version test.
There was a problem hiding this comment.
Thanks for the description.
whether Reline is truly compatible with this version
The first goal is to make $if version directive work, and I think we can't say we officially support 8.2. It's better to define this in config.rb as a private constant for now.
class Reline::Config
...
COMPATIBLE_READLINE_VERSION = '8.0'
private_constant : COMPATIBLE_READLINE_VERSION
endFor now, I think specifying major version 8.0 is better. (Until checking Reline really supports feature added between 8.0 and 8.2)
`mode`, `term`, `version`, and variable Specification: <https://tiswww.cwru.edu/php/chet/readline/readline.html#Conditional-Init-Constructs> readline implementation: <https://git.savannah.gnu.org/cgit/readline.git/tree/bind.c?h=readline-8.2#n1255>
smmr0
force-pushed
the
inputrc-if-directive
branch
from
ee5384c
to
1e0a9d2
Compare
| @@ -227,18 +239,36 @@ def read_lines(lines, file = nil) | |||
| end | |||
|
|
|||
| def handle_directive(directive, file, no) | |||
| directive, args = directive.split(' ') | |||
| directive, args = directive.split(/\s+/, 2) | |||
| args, _comment = args.split(/\s+#\s*/, 2) if args | |||
There was a problem hiding this comment.
Readline only supports /^#.*/ style comment and /^\$if\s+version[comparator][value]\s*#.*/ style comment.
It may look like supporting comment, but as far as readling gnu readline source code, it does not. $if mode = foo # comment is same as $if mode=foo garbage.
| elsif %w[Ruby Reline].include?(args) # application name | ||
| true | ||
| elsif %w[= == !=].include?(stripped_operator) # variable | ||
| parse_variable(lhs, rhs).all? do |var_name, rhs_value| |
There was a problem hiding this comment.
parse_variable might return nil, it wrongly compares variables which readline does not.
I think validating lhs and then directly compare to instance variable is enough.
if VARIABLE_NAMES.include?(lhs) && get_value_as_string(lhs) == rhs.downcase# readline 8.2
# on will set bell-style to audible
set bell-style on
# this is evaluated to true
$if bell-style = audible
# this is evaluated to false. "audible" != "on"
$if bell-style = on
# invalid value will set to off
set colored-completion-prefix on
set colored-completion-prefix offfff
# evaluated to true.
$if colored-completion-prefix = off
# evaluated to false because "off" != "offfff"
$if colored-completion-prefix = offfff| @@ -256,81 +286,103 @@ def handle_directive(directive, file, no) | |||
| end | |||
| end | |||
|
|
|||
| def bind_variable(name, value) | |||
| def parse_variable(name, value) | |||
There was a problem hiding this comment.
Can you revert this change? I think this change is not needed.
I added a comment where parse_variable is used.
| @@ -1,3 +1,4 @@ | |||
| module Reline | |||
| VERSION = '0.3.8' | |||
| COMPATIBLE_READLINE_VERSION = '8.2' | |||
There was a problem hiding this comment.
Thanks for the description.
whether Reline is truly compatible with this version
The first goal is to make $if version directive work, and I think we can't say we officially support 8.2. It's better to define this in config.rb as a private constant for now.
class Reline::Config
...
COMPATIBLE_READLINE_VERSION = '8.0'
private_constant : COMPATIBLE_READLINE_VERSION
endFor now, I think specifying major version 8.0 is better. (Until checking Reline really supports feature added between 8.0 and 8.2)
| condition = true if args == 'Reline' | ||
| end | ||
| lhs, operator, rhs = args.partition(/\s*(==|=|!=|<=|>=|<|>)\s*/) | ||
| stripped_operator = $1 |
There was a problem hiding this comment.
I think stripped_operator is not needed. operator is enough.
|
@smmr0 Hi. Just checking in on this PR since it's been quiet for a while. Do you plan to continue working on it? If not, I'd be happy to take over. Let me know! |
The remaining tests are
mode,term,version, and variable.Links:
It looks like there were plans to implement these tests, but those plans were abandoned.