Conversation
DCzajkowski
left a comment
There was a problem hiding this comment.
Are you planning to add tests for these features? We should probably require tests on all future PRs and invest some more time in heavy testing.
src/main/modules/SnippetsManager.js
Outdated
| _addNewFields() { | ||
| const newSnippets = this.store.get('snippets').map(s => { | ||
| s.active = s.active || true | ||
| s.regex = s.regex || false |
There was a problem hiding this comment.
These won't work. If someone sets active to false, you would map it to true 😛
There was a problem hiding this comment.
The === undefined check would be appropriate here
There was a problem hiding this comment.
omg i am so embarrassed right now xd
src/main/modules/SnippetsManager.js
Outdated
| } | ||
|
|
||
| _addNewFields() { | ||
| const newSnippets = this.store.get('snippets').map(s => { |
There was a problem hiding this comment.
I'd personally either inline this variable or just call it const snippets. Just a personal preference.
src/main/modules/SnippetsManager.js
Outdated
| this._addNewFields() | ||
| } | ||
|
|
||
| _addNewFields() { |
There was a problem hiding this comment.
_addNewFields -> _ensureAllFieldsArePresent? "Add new fields" implies new fields are always added, where in fact the function is just ensuring everything is set properly.
|
|
||
| Use Regular Expression | ||
| </label> | ||
| active |
There was a problem hiding this comment.
"active" -> "Active" to preserve Title Case style.
|
I don't really have experience with unit tests in js. To be honest i wasn't gonna do that, although i see clearly that we are missing tests and we should start to write them. I have to catch up on this fast! |
This pull adds activeness of snippets.
From now on you will be able to deactivate certain snippet and activate it again!