Compact Message Storage #664
Conversation
|
What do you think of this idea? |
|
Nice, it makes more sense imho to have What method would be made public? |
|
They are both public, but |
| @messagesProject.delete(linter) | ||
| @emitter.emit 'did-change-project-messages', @messagesProject | ||
| @views.render() | ||
| console.warn("Linter::deleteProjectMessages is deprecated, use Linter::deleteMessages instead") |
There was a problem hiding this comment.
Shall we use same system of Atom? (https://atom.io/packages/deprecation-cop)
There was a problem hiding this comment.
I was gonna switch to that before merge, these warns were temp. But still thanks for the link 👏
There was a problem hiding this comment.
Thank's for having so much time for doing this 💯
|
|
||
| getMessages: -> | ||
| @messages | ||
| @linter.subscriptions.add @linter.onDidChangeMessages => |
There was a problem hiding this comment.
Wait, what? Why are you adding a Disposable to @linter instead of keeping it local?
There was a problem hiding this comment.
It's still a work in progress, I am making changes one by one. and this one is planned.
|
I'm a little bit concerned that So let me propose something a little bit different: have a singleton If you choose to go singleton module route, you don't need to thread it around even, just Does that sound reasonable? P.S. singleton module does have its pros and cons, so I won't push too hard for this particular pattern. General principle of having a separate |
|
@lierdakil I agree with you, but I don't think that's the scope of the PR. We can do it after, I like the idea of having a store for these. |
|
@iam4x, if @steelbrain is doing it now, he might as well do it right the first time around. There is a whole lot of other possible code enhancements, so it's not like we'll run out of PR material any time soon. Enforcing encapsulation and module separation at the early stage is important, because after a while, refactoring horribly entangled parts out becomes a huge undertaking, and can lead to unavoidable API breakage, which is bad, for obvious reasons. That said, it's not my repo (and I prefer it this way), so you do have a final say in the matter. |
|
Okay guys so I did some profiling with 266 messages and here's the results so the thing that's taking most of our time is the regex parser we use, we might wanna switch to pure js regex (/cc @AsaAyers (it's coffeelint)). other than that, It's just appendChild and render Custom Element components (Message::attachedCallback) which is a DOM thing that we can't optimize. |
|
I need some testers people, our automated test coverage isn't very good. and I am gonna target it in my next PR where we make the modules work in isolated env. |
|
(wrong button click) |
add go to previous error
|
@steelbrain edit: that |
|
If you really want to profile the right thing, you should use a CLI linter so the internals of how the linter runs doesn't show up in your profile |
| @@ -1,7 +1,7 @@ | |||
| {Range} = require('atom') | |||
|
|
|||
| Helpers = module.exports = | |||
| validateResults: (results) -> | |||
| validateMessages: (results) -> | |||
There was a problem hiding this comment.
Here's the reason the tests are broken!
|
Merging in to keep the pace of progress (been almost a week since this is open), there have been zero known non-fixed bugs in this PR. |
{get, set, delete, observe}ProjectMessagesin favor of{get, set, delete, observe}MessagesKnown bugs