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

Can't use EOF heredoc #5403

Closed
chrisokamoto opened this issue Jan 5, 2018 · 11 comments · Fixed by #5712
Closed

Can't use EOF heredoc #5403

chrisokamoto opened this issue Jan 5, 2018 · 11 comments · Fixed by #5712

Comments

@chrisokamoto
Copy link

chrisokamoto commented Jan 5, 2018

In the docs it says:

This cop checks that your heredocs are using meaningful delimiters. By default it disallows END and EO*, and can be configured through blacklisting additional delimiters.

But in the AllowedAcronyms' table, it says that EOF is allowed, but in fact, it isn't. When I try to use EOF, it says:

Naming/HeredocDelimiterNaming: Use meaningful heredoc delimiters. EOF

But I am not sure if I misunderstood it.

$ rubocop -V
0.52.1 (using Parser 2.4.0.2, running on ruby 2.4.1 x86_64-linux)
@mikegee
Copy link
Contributor

mikegee commented Jan 5, 2018

It looks like it is trying to disallow "END" and "EO[A-Z]" like the message says.

I found "EOF" in an AllowedAcronyms list, but I think that list applies only to Naming/FileName, not Naming/HeredocDelimiterNaming.

@mcfisch
Copy link
Contributor

mcfisch commented Mar 21, 2018

The rule doesn't allow anything starting with EO or containing END. The rule isn't specific enough, and therefore actually doesn't really allow ANY meaningful delimiters. Changing it to this fixes it:

  Blacklist:
    - ^END$
    - !ruby/regexp '/^EO[A-EG-Z]{1}$/'

@mcfisch
Copy link
Contributor

mcfisch commented Mar 21, 2018

This would block all 3-char delimiters starting with EO as well as END, but still allow EOF. Will create PR for that.

mcfisch pushed a commit to mcfisch/rubocop that referenced this issue Mar 21, 2018
mcfisch pushed a commit to mcfisch/rubocop that referenced this issue Mar 22, 2018
[Fix rubocop#5403] consolidating the patterns into one regexp
mcfisch pushed a commit to mcfisch/rubocop that referenced this issue Mar 22, 2018
@mcfisch
Copy link
Contributor

mcfisch commented Mar 22, 2018

Created #5712 to fix this. Tests passed, also works fine in my local ~/.rubocop.yml.

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 22, 2018

EOF was intentionally disallowed as a non-meaningful delimiter of this cop. The acronym list is for an unrelated cop. 🙂

If you want to use EOF in your project, the right way is to use the local configuration option. 😉

@mcfisch
Copy link
Contributor

mcfisch commented Mar 22, 2018

Ok, so it isn't related to the acronyms list, they just reside next to each other because they belong to the same category. Ok, I'll change the code accordingly.

However, the current implementation doesn't allow to have any delimiter starting or even containing EO[A-Z] or END, and I can't imagine this is on purpose, is it?

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 23, 2018

The rationale is it's an "empty" delimiter, which doesn't help provide context for what the contents of the string is. Compare:

foo = <<-EOF
  ...
EOF

to:

foo = <<-SQL
  ...
SQL

or:

foo = <<-ERROR
  ...
ERROR

In both the latter cases, you get an idea of what this string is, even without looking at the content or the variable name. In the case of SQL, most editors will also apply code highlighting to the string content.

@mcfisch
Copy link
Contributor

mcfisch commented Mar 28, 2018

The problem with the current definition is, that it even faults something like this just because it contains the banned pattern:

foo = <<-ENDPOVERTY
  ...
ENDPOVERTY

as well as

foo = <<-ISLEOFMAN
  ...
ISLEOFMAN

My change would limit it to just ban a solely used END or any other 3-char delimiter starting with EO.

@mcfisch
Copy link
Contributor

mcfisch commented Mar 28, 2018

I've changed the pattern to include EOF as well.

@Drenmi
Copy link
Collaborator

Drenmi commented Apr 4, 2018

The problem with the current definition is, that it even faults something like this just because it contains the banned pattern.

Oh. Yes. This is definitely not intended. Good catch! 🙂

@mcfisch
Copy link
Contributor

mcfisch commented Apr 23, 2018

Thx for the merge, couldn't get back to it earlier.

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 a pull request may close this issue.

4 participants