-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Recognize whole triple-quoted string with regex instead of partial #114
Conversation
Fixes JuliaEditorSupport#15 Feels kind of silly to make such a small change after just working on this, but JuliaEditorSupport#113 made this fix much more obvious.
These are changes unrelated to JuliaEditorSupport#15 that just make the code a bit more readable. This commit should be reviewed separately. Since the julia group was declared with a :prefix of julia- all of these customization variables will be in the julia group anyway. Also, since julia-mode didn't declare it's group, an anonymous julia-mode group was getting created for faces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the incidental style fixes too.
@@ -48,19 +48,15 @@ | |||
|
|||
(defcustom julia-indent-offset 4 | |||
"Number of spaces per indentation level." | |||
:type 'integer | |||
:group 'julia) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why the :group
was removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure (there's also justification in the commit message). Since the group was declared with :prefix "julia-"
, any defcustom
that starts with "julia-" will automatically be in the group. So the :group
is merely redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I missed that. I didn't know about this feature of defcustom
. Is this documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like it. The manual is the only place I see :prefix
mentioned, and it doesn't talk about implicitly including variables having the prefix.
https://www.gnu.org/software/emacs/manual/html_node/elisp/Group-Definitions.html
I'm aware of the feature because eglot makes use of it. Quickly checked few of emac's builtin progmodes and perl makes use of it, too. If it's an implementation detail, it's one that's at least relatively widely relied upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it has nothing to do with :prefix
. Bad assumption on my part. The elisp manual says:
If a defcustom does not specify any :group, the last group defined with defgroup in the same file will be used. This way, most defcustom do not need an explicit :group.
|
||
(defface julia-macro-face | ||
'((t :inherit font-lock-preprocessor-face)) | ||
"Face for Julia macro invocations." | ||
:group 'julia-mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here, and the one below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was actually deleted because julia-mode
was a group only created anonymously by these defgroup invocations. I'm assuming it was a mistake and they were meant to be members of julia
group instead.
Thank you very much for this. I actually prefer changes in small pieces so it is no problem. @giordano, can you please review? |
Don't merge this. It has some strange transient behavior I don't understand. When font-locking a buffer from scratch, it works exactly as expected, but when I insert a triple-quoted string into a buffer, everything after the triple-quoted string gets font-locked as a string for some reason until I e.g. |
Closing in favor of #133 |
Fixes #15
Feels kind of silly to make such a small change after just working on
this, but #113 made this fix much more obvious.