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

Credo.Check.Readability.StringSigils should not warn on multiline string #611

Closed
chulkilee opened this issue Dec 5, 2018 · 5 comments
Closed

Comments

@chulkilee
Copy link

chulkilee commented Dec 5, 2018

Environment

  • Credo version (mix credo -v): 1.0.0
  • Erlang/Elixir version (elixir -v): 1.7.4
  • Operating system: mac

What were you trying to do?

Use multiline string to wrap JSON string

"""
{
  "hello": "world"
  "foo": "bar"
}
"""
|> Jason.parse()

Expected outcome

No error from Credo.Check.Readability.StringSigils since it's already readable :)

Note that ~s cannot be used in that case since it does not remove leading spaces (like """).

Actual outcome

Credo.Check.Readability.StringSigils is not happy with it

More than 3 quotes found inside string literal, consider using a sigil instead.

@chulkilee
Copy link
Author

I'm curious whether that info (string is multiline or not) is available to Credo.

rrrene pushed a commit that referenced this issue Mar 7, 2019
@rrrene
Copy link
Owner

rrrene commented Mar 8, 2019

@chulkilee You are right, this should not warn for multi-line strings ("heredocs"). I'm not sure why it does, since I was not able to reproduce this.

Could you provide a file as a test-case? (oh, and sorry, I completely missed your issue 😔)

@chulkilee
Copy link
Author

Updated the example - it happens when I pass such string into pipe.

rrrene pushed a commit that referenced this issue Mar 9, 2019
@rrrene
Copy link
Owner

rrrene commented Mar 9, 2019

Got it. Thanks! Will work on a fix 👍

@rrrene rrrene closed this as completed in 7a92ac2 Mar 9, 2019
@rrrene
Copy link
Owner

rrrene commented Mar 9, 2019

@chulkilee
Thanks for reporting this 😀 It is now fixed on master.

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

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

No branches or pull requests

2 participants