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 #[DELIM[ … ]DELIM] syntax for string literals #1379

Merged
merged 4 commits into from
Sep 15, 2017

Conversation

Kodiologist
Copy link
Member

@Kodiologist Kodiologist commented Aug 10, 2017

@Kodiologist
Copy link
Member Author

@gilch @kirbyfan64 Is there any reason you guys aren't reviewing this? I thought we settled on the Lua style.

@gilch
Copy link
Member

gilch commented Sep 1, 2017

I thought we settled on the Lua style.

I thought we did, or I did, at least. There are competing priorities, that's all. I can make this a priority since you think it's important.

This can come later, but we'll want to at least update hy-mode for the new syntax, and vim-hy too, but no-one seems to be maintaining that. Since hy-mode isn't settled even for what we've got so far, I've been de-prioritizing syntax changes that would break it compared to other things.

But even before then, both emacs and vim should mostly highlight properly if Hy files if we pick #["[...]"] as the delimiters. I'd also like to get a basic linter going for .hy files #1398. Parlinter looks promising, but it won't work with the new syntax generally, just with #["[, which it will recognize as a string inside []. (The other proposed string syntax options probably can't even do that much.) We're not the only Lisp with this kind of problem, so we can probably get that fixed. I'd even translate it to Python and do it myself eventually, but priorities.

@Kodiologist
Copy link
Member Author

There are competing priorities, that's all. I can make this a priority since you think it's important.

My own philosophy is that I should try to review PRs (that are ready for review, to my knowledge) before anything else (like writing issues, working on my own PRs, etc.), because PR reviewing seems to be the primary bottleneck for Hy development.

@gilch
Copy link
Member

gilch commented Sep 1, 2017

=> (. '#[foo[bar]foo] hashstring_delim)
from hy import HyString
HyString('bar').hashstring_delim
Traceback (most recent call last):
  File "c:\users\me\documents\github\hylang-hy\hy\importer.py", line 201, in hy_eval
    return eval(ast_compile(expr, "<eval>", "eval"), namespace)
  File "<eval>", line 1, in <module>
AttributeError: 'HyString' object has no attribute 'hashstring_delim'

I can't seem to access the delimiter from the model. Is the compiler stripping it out somewhere?

@gilch
Copy link
Member

gilch commented Sep 1, 2017

Looks like the macros can get to it though.

=> (defmacro foo [s] (. s hashstring_delim))
import hy
hy.macros.macro('foo')((lambda s: s.hashstring_delim))
<function <lambda> at 0x000001BDD82476A8>
=> (foo #[x[]x])
'x'
'x'

@Kodiologist
Copy link
Member Author

Kodiologist commented Sep 1, 2017

I think what you're seeing in the first example is a limitation in how quoting works. The generated code is HyString('bar'), which doesn't set the attribute. It seems clear that this isn't right, but I'm not sure what needs to be fixed.

@Kodiologist
Copy link
Member Author

I think HyASTCompiler._render_quoted_form needs an upgrade to set the attribute. I'm looking into it now.

@Kodiologist
Copy link
Member Author

Done.

@gilch
Copy link
Member

gilch commented Sep 1, 2017

Tested too, that looks good.

Seeing those elif isinstance statements in a super-long method makes me want to move that logic to methods in the individual Hy models themselves. It's another issue though. For now, it's more consistent this way. The refactor should be in #1399, or at least wait until after that's merged.

Now that the delimiter is a keyword argument, I think I should put it in the HyString repr in #1360, at least when it doesn't have the default value of None. (This will have to be merged first, so I can rebase.) But perhaps it should have a shorter name than hashstring_delim now? It's not kwonly, so I could just put it second positionally, but it might be clearer with the word.

We also need news and docs, which should also specify that some number of ='s are the preferred delimiters when #[[ won't do, and there's not a specific reason to use something else.

@Kodiologist
Copy link
Member Author

Kodiologist commented Sep 1, 2017

Seeing those elif isinstance statements in a super-long method makes me want to move that logic to methods in the individual Hy models themselves. It's another issue though. For now, it's more consistent this way.

Agreed on both counts.

But perhaps it should have a shorter name than hashstring_delim now?

It doesn't make a difference to me. hashstring_delim just seemed like the most obvious choice. Is there any particular alternative you'd prefer?

We also need news and docs

Will do.

which should also specify that some number of ='s are the preferred delimiters when #[[ won't do, and there's not a specific reason to use something else.

I disagree. I don't want to count =s. Let's let the reader decide, or at least defer this debate until a style-guide PR.

@gilch
Copy link
Member

gilch commented Sep 1, 2017

I don't want to count =s.

I don't want to count more than three, but how often does that come up?

at least defer this debate until a style-guide PR

OK, I guess.

hashstring_delim just seemed like the most obvious choice. Is there any particular alternative you'd prefer?

I didn't have a particular alternative in mind, but I guess brackets could work.

@kirbyfan64, anything else you'd like to add? Do you have a preference for the HyString reprs?

@Kodiologist
Copy link
Member Author

Done.

@Kodiologist
Copy link
Member Author

@gilch This good? I wasn't sure whether you wanted me to change hashstring_delim to brackets or you were just thinking out loud.

@gilch
Copy link
Member

gilch commented Sep 7, 2017

Go ahead and change it to brackets.

@Kodiologist
Copy link
Member Author

@gilch Done.

hy/compiler.py Outdated
l = [HySymbol(name), form]
if form.brackets is not None:
l.extend([HyKeyword(":brackets"), form.brackets])
return imports, HyExpression(l).replace(form), False
Copy link
Member

Choose a reason for hiding this comment

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

This is minor, but I'd prefer to avoid single-character names that can be confused with other characters in Python, like lI1 or 0O. These are distinguishable in a good programmer font, but you might have to squint. This is the kind of thing flake8 should be able to catch.

@gilch
Copy link
Member

gilch commented Sep 7, 2017

The implementation seems good now.

I don't like the name "hashstring" it makes it sound like it has something to do with hash functions, but it doesn't. It's just from the # character. We could call them "bracket strings", unless you have a better idea?

@Kodiologist
Copy link
Member Author

Kodiologist commented Sep 7, 2017

"Bracket string" seems as good as anything. I will likely get around to the mild inconvenience of editing this tomorrow.

@Kodiologist
Copy link
Member Author

Both changes made.

@gilch gilch self-requested a review September 8, 2017 22:49
@refi64
Copy link
Contributor

refi64 commented Sep 15, 2017

Glad to see this finally getting in!

@refi64 refi64 merged commit 44e5ded into hylang:master Sep 15, 2017
@ekaschalk
Copy link
Contributor

Bracket string literals are now fully supported in hy-mode master branch.

@Kodiologist Kodiologist deleted the lua-str-literal branch January 20, 2019 14:41
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.

4 participants