Suggested changes #1752
Suggested changes #1752
Conversation
salbertson
force-pushed
the
suggested-changes
branch
from
75d15b9
to
bb5d7af
Compare
doc/SECURITY.md
Outdated
| We do not save any of your code in Postgres. | ||
| We do store the contents of the files to review temporarily in Sidekiq, and it | ||
| gets cleared out as soon as the job is processed. | ||
| We store single lines of your code in Postgres, which is encrypted and encoded |
There was a problem hiding this comment.
How about explaining this a bit more:
We only store single lines of code that we detected a linter violation
Or something? So we explain exactly which lines we store.
salbertson
force-pushed
the
suggested-changes
branch
from
7d42a53
to
fa327d7
Compare
salbertson
force-pushed
the
suggested-changes
branch
from
c80cf7a
to
b8ddf22
Compare
app/services/suggest_changes.rb
Outdated
| when /Put a comma after the last parameter of a multiline method call/ | ||
| self.suggestion = violation.source << "," | ||
| when /Missing semicolon/ | ||
| self.suggestion = violation.source << ";" |
There was a problem hiding this comment.
Dos violation.source << ";" mutate the violation.source? Maybe + or interpolation is safer?
There was a problem hiding this comment.
Yes, good call. Done.
| suggested changes. The data is encrypted and encoded similar to tokens using | ||
| `ActiveSupport::MessageEncryptor`. We also store the contents of the files to | ||
| review temporarily in Redis, but that is removed as soon as the Sidekiq job is | ||
| processed. |
| result = suggest_changes.call | ||
|
|
||
| expect(result).to eq <<~COMMENT.chomp | ||
| Put a comma after the last parameter of a multiline method call.<br>```suggestion |
There was a problem hiding this comment.
What do you think about adding \n before or after <br> in the code, so these assertions don't look so whacky?
There was a problem hiding this comment.
Not sure what you mean. If I do that then the tests will fail because the messages won't match. 😕
There was a problem hiding this comment.
I'm referring to this line: https://github.com/houndci/hound/pull/1752/files#diff-326c371081e0ec9a5f273d1c45b64541R20
If we add an \n, like:
messages + "\n<br>```suggestion\n#{suggestion}\n```"then this test line would look like:
Put a comma after the last parameter of a multiline method call.
<br>```suggestionThere was a problem hiding this comment.
@gylaz works for me, done.
|
|
||
| def apply_suggestion(message) | ||
| case message | ||
| when /Trailing whitespace detected/ |
There was a problem hiding this comment.
@gylaz we need to figure out a better way to generate these suggestions.
There was a problem hiding this comment.
Yep, though not sure how "smart" we want to get about these.
salbertson
force-pushed
the
suggested-changes
branch
from
fff3d3c
to
7d17f02
Compare
salbertson
force-pushed
the
suggested-changes
branch
from
7d17f02
to
d48f119
Compare
No description provided.