Conversation
|
Generally, if you have the bandwidth (and I'd argue that even if you're stretched for time this is still worth doing), once you get your code into a "ready for review state", do the same for your commits. It makes a review much easier if you build a coherent story out of your commits. Added benefit is that it forces you to reason about your work in a nicely structured way. Hypothetical series of commits:
|
addon-chrome/ChromePlacesProvider.js
Outdated
| blockedUrls.map((blocked) => blocked.url).indexOf(item.url) === -1); | ||
| } | ||
|
|
||
| static _mergeLinks(r1, r2) { |
There was a problem hiding this comment.
This is actually merging bookmark GUIDs from r2 (assuming r2 is a list of bookmarks?) into r1. Consider renaming this method and variable names to make this explicit. Also, method comments.
|
OK, let's call it "first pass". It's looking good! I didn't comment much on formatting, naming, etc - let's address some structural concerns first. |
dan-l
force-pushed
the
master
branch
5 times, most recently
from
7a9a95f to
fea63fc
Compare
addon-chrome/ChromePlacesProvider.js
Outdated
| }); | ||
| } | ||
|
|
||
| static _mergeHistoryBookmark(hist, bookmarks) { |
… compose them as needed, Use lodash utility method for stability, Add comments for clarifications
addon-chrome/ChromePlacesProvider.js
Outdated
| const searchResultRegex = new RegExp(SEARCH_RESULT_REGEX); | ||
| const localhostRegex = new RegExp(LOCALHOST_REGEX); | ||
| const browserResourceRegex = new RegExp(BROWSER_RESOURCE_REGEX); | ||
| const filterRegex = new RegExp(searchResultRegex.source + "|" + localhostRegex.source + "|" + browserResourceRegex.source); |
There was a problem hiding this comment.
on regex bugs: http://stackstatus.net/post/147710624694/outage-postmortem-july-20-2016
and relevant discussions: https://news.ycombinator.com/item?id=12131909
;-)
2 participants
No description provided.