-
Notifications
You must be signed in to change notification settings - Fork 178
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
Cleanup Message Registry #738
Cleanup Message Registry #738
Conversation
…ade-message-registry
Everything works, except the count on the bottom and line messages, working on them. |
…ade-message-registry
I am done with it. Tests seem to pass and everything seems okay. |
…ade-message-registry
…ade-message-registry
@@ -35,6 +33,10 @@ class EditorLinter | |||
onDidDestroy: (callback) -> | |||
return @emitter.on('did-destroy', callback) | |||
|
|||
destroy: -> | |||
@emitter.emit('did-destroy') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be at the end of the function since it is did-destroy
not will-destroy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually called after TextEditor is destroyed, not before it, here we are emulating the same event, to support the linter:toggle
command and emulate a TextEditor destruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steelbrain, he means that it should be done after @deactivate() is called so it represents that deactivate succeeded correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it'll change anything, but I'll still do it 👏
text = message.text || message.html | ||
toReturn = message.type + '-' + message.filePath + '-' + (message.range?.serialize?()) + '-' + text | ||
return toReturn.toLowerCase() | ||
shouldTriggerLinter: (linter, bufferModifying, onChange, scopes) -> | ||
# Trigger fly linters on save, but not save linters on fly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is confusing. It should probably refer to "fly linters" as "on-the-fly linters".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
getMessages: -> | ||
return @messages.getAll() | ||
return @messages.publicMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove explicit return
render: -> | ||
@messages = @linter.messages.getAllMessages() | ||
render: (messages) -> | ||
@messages = @classifyMessagesByLine(@classifyMessages(messages)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you ever call classifyMessagesByLine
or classifyMessages
separate from each other? If not, then one should call the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if @updated | ||
@updated = false | ||
publicMessages = [] | ||
@messages.forEach (messages) -> publicMessages = publicMessages.concat(messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should probably be:
@messages.forEach (message) -> publicMessages.push(message)
Singular name on the anonymous function parameter because it is passing in a single message. And push
probably doesn't needlessly allocate another object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@messages
is actually a array<Linter, array<Message>>
so we need to iterate over editorMessages and then get the messages and then concat them as one array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steelbrain, we need to rename @messages
to something else then. Perhaps @linterResponses
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Conflicts: lib/helpers.coffee
…ade-message-registry
…ade-message-registry
@@ -110,6 +112,6 @@ class Linter | |||
@views.destroy() | |||
@linters.deactivate() | |||
@commands.destroy() | |||
@messages.destroy() | |||
@messages.deactivate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should really rename all these things dispose so that we are implementing the disposable interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then as we build them we can just register them into a composite disposable and just dispose it.
🚢 after adding tests. |
@park9140 everything that was introduced in this PR has tests for it, I'll be adding tests for other stuff incrementally. |
…e-registry Cleanup Message Registry
To be merged into #737
Fixes #716
Fixes #650
Ref: #708
Reviewers