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

Fix nano-modeline--base-face #53

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

Conversation

aaronjensen
Copy link
Contributor

Unless for some reason this function is supposed to return a single item list, I think this should be cadr, rather than cdr

@rougier
Copy link
Owner

rougier commented Jun 19, 2023

Thansk, but I'm not sure this is correct. We want to keep the list such that it can be inherit inside a face

@aaronjensen
Copy link
Contributor Author

I'm not sure I follow. The only usage of it is here:

https://github.com/rougier/nano-modeline/blob/master/nano-modeline.el#L259

If it appropriate to set a list of faces to the face property?

If so, maybe it's fine. I ran into some issues with it I thought, but it's likely they were actually caused by my other issue: #54

@aaronjensen
Copy link
Contributor Author

I think there was also an instance of the code where it would prepend a face to the faces, and you'd end up with a list like this: (some-face (the-base-face)), rather than (some-face the-base-face). Again, if that's valid then there's no issue here.

@rougier
Copy link
Owner

rougier commented Jun 19, 2023

I'm not sure at all actually. I'll need to test your fix a bit first. One question though, there are other instances where I use the cdr (for example in nano-modeline-face but you did not fix this one. Any reason?

@aaronjensen
Copy link
Contributor Author

I looked at it. I don't recall, but it was either used in a place that assumes it returns a list or it works against a cons and not a list, so the cdr is actually an item. The issue is the cdr of a list is a list, so it wasn't clear that that was actually the intent here.

@rougier
Copy link
Owner

rougier commented Jun 19, 2023

Unfortunately, I don"t remember very well. I think one of the motivation was for the :inherit that can accept an arbitrary list of feature.

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.

2 participants