monsanto / readline-complete.el

autocomplete in shell mode buffers
67 stars 16 forks source link

Add warning about bug in some emacs versions #17

Open ian-kelling opened 9 years ago

ian-kelling commented 9 years ago

11 is fixed upstream. Add warning for affected versions and call it fixed.

monsanto commented 9 years ago

Could you also add the warning to README.md? Otherwise looks good to me nm see comment

ian-kelling commented 9 years ago

I think I was a bit tired, didn't fully think this through. I've updated it to use patch version, and got the right version.

ian-kelling commented 9 years ago

... now.

ian-kelling commented 9 years ago

Addressed your comments, except, normalizing patch-ver to 0 won't work. It would include 0 when I test <= and exclude when I test >=, but I'd like to include it on both conditions. I couldn't think of a better way to do it, without making things overly complicated / verbose.

monsanto commented 9 years ago

I think you are mistaken:

(if patch-ver (>= 91 patch-ver) t) -->
(if nil (>= 91 nil) t) -->
t

(>= 91 patch-ver) -->
(>= 91 0) -->
t

and

(if patch-ver (<= patch-ver 50) t) -->
(if nil (<= nil 50) t) -->
t

(<= patch-ver 50) -->
(<= 0 50) -->
t
ian-kelling commented 9 years ago

Heh, we were both right.

(>= 91 patch-ver)

should be

(>= patch-ver 91)

However, I think you were right to say it's not worth it. I thought, I bet someone else did this already, and they did: http://www.emacswiki.org/emacs/versions.el, and if it's not worth including a library (which I don't think it is), it's not worth having this code, because there are downsides of potential bugs, added complexity for new contributors, etc. I've updated the pull request to be just the comment.

monsanto commented 9 years ago

Fair enough. Just add it to the README as well per my first comment and I will merge :)

monsanto commented 9 years ago

you alive @ian-kelling? i'll amend your commit myself if you don't do it by next time i check in.