avoid duplicates #158

ingorichter · 2017-09-26T06:23:19Z

This is a solution for #114

This enhancement provides 3 different options to wrangle tabs

allow duplicates (current behavior)
exact URL match
match host and title
This is the current behavior and it will be the default if the user doesn't change it.
Avoid a duplicate wrangled tab if there is already a wrangled tab in the list with the same exact URL. If there is already a wrangled tab, then we push it up to the top of the list and update the timestamp
Avoid a duplicate wrangled tab if there is already a wrangled tab in the list with the same host and the same title. If there is already a wrangled tab, then we push it up to the top of the list and update the timestamp

- added tests for Settings - add test for wrangleTabs - replace underscore with lodash because of undefined error when testing tabmanager

added some tests and fixed the failing once

change pattern for tests folder to avoid 2 consecutive runs added new filter to find tab position by hostname and title added tests added new component for tab wrangle option

…nto ingo/avoid-duplicates

…wrangled and added to the wrangled tab list

- fixed the test description

…nto ingo/avoid-duplicates

- added new description for the wrangle options

- fix broken test case

- fix broken test

ssorallen

Thanks for the PR. Can you check the Lodash dependency and whether this works with a fresh node_modules directory from a yarn install? If you need a single function from Lodash you can include a small portion of Lodash.

ssorallen · 2017-09-26T13:05:19Z

app/js/TabWrangleOption.js

+    super(props);
+
+    this.state = { selectedOption: this.props.selectedOption };
+    this.handleChange = this.handleChange.bind(this);


Prefer property initializers instead of bind in the constructor:

handleChange = (e) => { ... };

ssorallen · 2017-09-26T13:09:56Z

app/js/tabmanager.js

@@ -1,7 +1,10 @@
 /* @flow */
 /* global TW */

-import _ from 'underscore';
+// import _ from 'underscore';


This doesn't seem correct. Were you testing replacing Underscore with Lodash? Lodash is not listed as a dependency.

Ah, I nearly forgot about that. I was using lodash instead of underscore, because lodash caused an initialization error when running the tests. I couldn't find a solution to it, so I switched to lodash.
And although lodash isn't mentioned in the dependencies it's hoisted through some other dependencies. :-/
The question is if there is no solution to the initialization error with underscore, if we should make lodash a proper dependency and get rid of underscore. What do you think about that?

Lodash is a second-level dependency only of another devDependency (babel uses it, for example). It's not used in released code, however. Using Lodash increases the .zip release file by 30KB.

I'm fine replacing Underscore with Lodash completely, but let's not ship both of them in the same .zip.

Agree, we should only have either one of them only. I'm going to have another look at the underscore init issue. If I can't find a simple solution, I'm going to swap underscore out for lodash.

This issue seems related to the testing problem: jashkenas/underscore#2152

I'm not sure it's worth spending much time on this. If dropping in Lodash works, I'm cool with that. Feel free to go full Lodash and skip any more debugging. If you do, please use yarn add / yarn remove to update the lock file appropriately and replace import _ from 'underscore'; everywhere with import _ from 'lodash';. TabWrangler's use of Underscore is limited, so problems from swapping should be limited.

Yeah, that looks exactly what I was seeing. I've swapped underscore with lodash and updated the lock file.

ssorallen · 2017-09-26T13:12:36Z

app/js/TabWrangleOption.js

+      selectedOption: e.target.value,
+    });
+
+    this.props.onChange(e);


This is passing the new value up the hierarchy, and it's being passed in via props. Why have state in this component at all?

I keep the state to reflect which radio button is pressed. If there is a better way to achieve the same, I'm happy to learn about it.

ssorallen · 2017-09-26T13:16:18Z

app/js/settings.js

@@ -85,7 +85,7 @@ const Settings = {
   * @see Settings.set


Can you add an a value in defaults at the top of this file for the new wrangleOption setting?

wrangleOption: 'WITH_DUPES',

ssorallen · 2017-09-27T03:06:33Z

Can you also run yarn flow and resolve the Flow errors that are present?

- replace underscore with lodash due to issues during test run init - add default for wrangle option in settings.js - fix all flow issues with new code

ingorichter · 2017-09-27T07:50:57Z

All flow errors are gone now, and I've addressed the review comments. I'm going to bed now. It's too late, and I hope that I didn't forget anything.

ssorallen · 2017-09-27T11:20:09Z

@ingorichter Thanks again for all of your work. I'll test this out locally and get it merged. If there's anything minor, I'll fix it in master.

ingorichter · 2017-09-27T17:52:19Z

Sounds good to me.
Thank you for the feedback. Perhaps we should add yarn flow to the travis config. It's easily forgotten, and it'll make reviews shorter by not doing an extra round and fix that issues.

ssorallen · 2017-09-28T04:54:49Z

Perhaps we should add yarn flow to the travis config.

Agreed, running Flow during the build step would be great. I can add that to the Gulpfile.

ingorichter added 18 commits August 29, 2017 00:04

- fixed bug in setmaxTabs (range check to allow 1..500) tabs

3787aff

- added tests for Settings - add test for wrangleTabs - replace underscore with lodash because of undefined error when testing tabmanager

the missing tests

ce03baa

rearrange tabs to avoid dupes

7f983ad

simplified findPositionByURL

b186d6a

added some tests and fixed the failing once

add describe as global name

e745f6a

change pattern for tests folder to avoid 2 consecutive runs added new filter to find tab position by hostname and title added tests added new component for tab wrangle option

add nodejs 8 for travis

be9d62a

Merge branch 'master' of https://github.com/tabwrangler/tabwrangler i…

cb4bdc8

…nto ingo/avoid-duplicates

add radio buttons

22e4eef

- save the tab wrangle settings

cd8c880

fix the broken test due to changes of toLocaleDateString in node 8.x

bb9ee02

added test for new tab wrangle options component

54ad2f8

make use of the wrangle option and change the behavior when a tab is …

d3eea46

…wrangled and added to the wrangled tab list

- define a default value for the tab wrangle option in settings

91318f2

- fixed the test description

Merge branch 'master' of https://github.com/tabwrangler/tabwrangler i…

43e33a8

…nto ingo/avoid-duplicates

- added new test

2006cd6

- added new description for the wrangle options

- merge upstream master

65639c8

- fix broken test case

- fix eslint error

f8d28e5

- remove obsolete code

d5811b7

- fix broken test

ssorallen requested changes Sep 26, 2017

View reviewed changes

- update babel config to support property initializer

b825d6f

- replace underscore with lodash due to issues during test run init - add default for wrangle option in settings.js - fix all flow issues with new code

ssorallen merged commit 8cec23f into tabwrangler:master Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid duplicates #158

avoid duplicates #158

ingorichter commented Sep 26, 2017

ssorallen left a comment

ssorallen Sep 26, 2017

ingorichter Sep 27, 2017

ssorallen Sep 26, 2017

ingorichter Sep 26, 2017

ssorallen Sep 27, 2017

ingorichter Sep 27, 2017

ssorallen Sep 27, 2017 •

edited

ingorichter Sep 27, 2017

ssorallen Sep 26, 2017

ingorichter Sep 27, 2017

ssorallen Sep 26, 2017

ingorichter Sep 27, 2017

ssorallen commented Sep 27, 2017

ingorichter commented Sep 27, 2017

ssorallen commented Sep 27, 2017

ingorichter commented Sep 27, 2017

ssorallen commented Sep 28, 2017

avoid duplicates #158

avoid duplicates #158

Conversation

ingorichter commented Sep 26, 2017

ssorallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssorallen Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssorallen commented Sep 27, 2017

ingorichter commented Sep 27, 2017

ssorallen commented Sep 27, 2017

ingorichter commented Sep 27, 2017

ssorallen commented Sep 28, 2017

ssorallen Sep 27, 2017 •

edited