Comments (8)
Hi, yes, we should definitely do something about this 👍
What do you think about putting a simple to_string
here:
Line 728 in 3fb97c5
We did something similar to :trigger
a couple of weeks ago, so this would be a great contribution for consistency as well ✌️
from credo.
Indeed, to_string
was my initial attempt at fixing the issue in the plugin. But it was not sufficient to solve the problem.
I ended up with this below, which certainly produces strings that credo can successfully work with.
But reading it now with fresh eyes, I don't like how it is transforming the input in the case where String.valid?/2
is false. Let me refine this a bit.
defp binary_to_string(binary) do
codepoints = String.codepoints(binary)
Enum.reduce(codepoints, fn w, result ->
cond do
String.valid?(w) ->
result <> w
true ->
<<parsed::8>> = w
result <> <<parsed::utf8>>
end
end)
end
from credo.
Mmh, I get the feeling that we are not yet finished analysing the problem (the StackOverflow post that this snippet seems to be copied from also left me no wiser) ...
Quick thoughts:
- we should definitely let plugin authors know with a clear error message that their
:message
is not valid, because it is not a valid string. - I am not sure that we can convert non-valid Binaries to valid Strings in a generalized manner, without producing even more confusing error message when than abstraction leaks.
- Why is this a problem to begin with? Does this have something to do with file encoding?
Let me know your thoughts.
from credo.
- Sure, we could do that in
format_issue
. We could make the message say "Warning: MyCredoPlugin tried to issue an invalid message" or something like that if eitherString.printable?/2
orString.valid?/2
is false for either:message
or:trigger
. Or even just raise in that case, maybe? But I don't think this is our best option. - The approach below retains the maximum amount of well-formatted information from
:message
&:trigger
, while replacing the invalid data the way the standard library does. I think this is the sweet spot. - Not file encoding, but data from a reporting system export that I copied into some test cases. That other system data turns out to be consistent, but ill-formed. I figure if it happened to me, it will happen to others at some point.
This replaces every invalid character with the same mark, "�" by default. So the valid surrounding characters will offer the user context to help.
As a user, I'd rather receive 90% of a credo message with an invalid character than 0%.
defp to_valid_string(binary) do
binary
|> to_string()
|> String.replace_invalid()
end
Here is the test case I've been working on in test/credo/check_test.exs
to demonstrate:
test "it should cleanse invalid messages" do
minimum_meta = IssueMeta.for(%{filename: "nofile.ex"}, %{exit_status: 0})
invalid_message = <<70, 111, 117, 110, 100, 32, 109, 105, 115, 115, 112, 101, 108, 108, 101, 100, 32, 119, 111, 114, 100, 32, 96, 103, 97, 114, 114, 121, 226, 96, 46>>
invalid_trigger = <<103, 97, 114, 114, 121, 226>>
issue =
Check.format_issue(
minimum_meta,
[
message: invalid_message,
trigger: invalid_trigger
],
:some_category,
22,
__MODULE__
)
refute String.valid?(invalid_message)
refute String.valid?(invalid_trigger)
refute String.printable?(invalid_message)
refute String.printable?(invalid_trigger)
assert String.valid?(issue.message)
assert String.valid?(issue.trigger)
assert String.printable?(issue.message)
assert String.printable?(issue.trigger)
assert issue.message === "Found misspelled word `garry�`."
assert issue.trigger === "garry�"
end
from credo.
In my mind, we should not assume too much about the data we're given (especially when reading the source code of String.replace_invalid
I am reminded of how much I do not know about encodings and binaries).
And: We can not use String.replace_invalid
, because it was just introduced in the current minor version of Elixir.
Checking and warning with String.valid?
on the :message
seems the only reasonable option to me.
Your example shows that even the :trigger
can be super idiosyncratic and since the trigger
is the stuff in the source that should be highlighted (because it is "triggering" the issue), we cannot transform that inside Credo, because then it literally loses its meaning.
from credo.
Cool. How about something like this? I tried to keep Credo's helpful conversational style, but I admit to a tendency to over-word things.
I do think it's nice when a message, when searched, yields exactly the right results, so I considered that in this proposed wording.
assert issue.message ===
"The Credo message you were meant to see here contained at least one invalid byte, so we could not show it to you."
assert issue.trigger === "The trigger for this Credo issue contained at least one invalid byte, so we could not show it to you."
from credo.
Why would we overwrite a message instead of just printing a warning?
I do think it's nice when a message, when searched, yields exactly the right results, so I considered that in this proposed wording.
What does this even mean? What "right results"?
from credo.
Why would we overwrite a message instead of just printing a warning?
Yes, of course. Definitely better. I outline a few approaches below.
What does this even mean? What "right results"?
Accurate search hits with minimum false positives. Users naturally search for the warning phrase verbatim.
Approach 1. Skip Malformed Issues and/or Malformed Messages
We could, for default output, skip malformed issues here, either by skipping the issue entirely or by skipping the invalid message, leaving the other issue elements to be output:
But then that leaves the question of what to do about the other outputs: flycheck, json, oneline, and sarif.
Approach 2. Filter Out Malformed Issues from Execution
Alternatively, we could filter out issues with invalid messages from Execution.get_issues/1
and get_issues/2
and get_issues_grouped_by_filename/1
:
Line 514 in 3fb97c5
But this seems strange to filter out issues at this level of abstraction due to an output concern. And lots of other code calls this function.
Approach 3. Smallest change and most simple
Since wrap_at/2
is where the handling of the invalid string raises, we could return an empty string when invalid, and perhaps warn, otherwise continue as normal:
credo/lib/credo/cli/output/ui.ex
Line 57 in 3fb97c5
This is the smallest surface area solution, with only two direct callers.
Regardless
To help plugin authors, I think regardless of which approach we take, we would add checks for invalid message and trigger values here:
Lines 183 to 204 in 3fb97c5
from credo.
Related Issues (20)
- Credo warning /test/add_credo_plugin_to_project.exs:121:6 HOT 2
- Code readability: Predicate function names should not start with 'is', and should end in a question mark. HOT 1
- Command line switch `--read-from-stdin` doesn't respect config files HOT 4
- Predicate callbacks from behavior are not ignored HOT 2
- Issue with Unused variables confused about the strategy HOT 1
- missing spec on unquoted function raise error HOT 3
- Credo.Check.Warning.WrongTestFileExtension displayed in non-test module in v1.7.4 HOT 8
- NoAmbiguousAliases fails on function param with name alias HOT 3
- "Most of the time you are using the multi-alias/require/import/use syntax..." with compile_env HOT 4
- False positive: spec in quoted HOT 3
- False Positive: Macro sigils with uppercase words HOT 4
- `UnusedKeywordOperation` check should ignore `Keyword.validate!/2` HOT 4
- Arguable advice from `Credo.Check.Refactor.AppendSingleItem` HOT 1
- Predicate function names should not start with 'is', and should end in a question mark flagged for :ets.whereis HOT 2
- Not ready for Elixir 1.16: a lot of 'warning: negative steps are not supported in Enum.slice/2 HOT 1
- Elixir 1.17 issues HOT 7
- Dependency divergence on compile HOT 1
- Unicode characters (arrows) that indicate priority of found issues not printed correctly HOT 1
- Credo.Check.Warning.Dbg: doesn't warn when using `&dbg/1`
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from credo.