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

Add warning about bug in some emacs versions #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add warning about bug in some emacs versions #17

wants to merge 1 commit into from

Conversation

ian-kelling
Copy link
Contributor

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

@monsanto
Copy link
Owner

monsanto commented Jul 6, 2015

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

@@ -135,10 +135,18 @@
;; editline instead of readline. Consider recompiling bash, python,
;; etc with readline support instead.

;; *** Note: emacs contained a bug from ~24.3.91 to ~24.5-rc3-fixed where
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your bug was reported for 25.0.50, so the bit about 24.5-rc3-fixed seems off to me? Or do you mean that is when the bug was introduced?

@ian-kelling
Copy link
Contributor Author

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
Copy link
Contributor Author

... now.

@ian-kelling
Copy link
Contributor Author

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
Copy link
Owner

monsanto commented Jul 7, 2015

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
Copy link
Contributor Author

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
Copy link
Owner

monsanto commented Jul 8, 2015

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

@monsanto
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants