{"action":"created","issue":{"url":"https://api.github.com/repos/Mottie/tablesorter/issues/830","labels_url":"https://api.github.com/repos/Mottie/tablesorter/issues/830/labels{/name}","comments_url":"https://api.github.com/repos/Mottie/tablesorter/issues/830/comments","events_url":"https://api.github.com/repos/Mottie/tablesorter/issues/830/events","html_url":"https://github.com/Mottie/tablesorter/pull/830","id":58903335,"number":830,"title":"Group: remove unnecessary complexity for further refactorings","user":{"login":"prijutme4ty","id":814984,"avatar_url":"https://avatars.githubusercontent.com/u/814984?v=3","gravatar_id":"","url":"https://api.github.com/users/prijutme4ty","html_url":"https://github.com/prijutme4ty","followers_url":"https://api.github.com/users/prijutme4ty/followers","following_url":"https://api.github.com/users/prijutme4ty/following{/other_user}","gists_url":"https://api.github.com/users/prijutme4ty/gists{/gist_id}","starred_url":"https://api.github.com/users/prijutme4ty/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/prijutme4ty/subscriptions","organizations_url":"https://api.github.com/users/prijutme4ty/orgs","repos_url":"https://api.github.com/users/prijutme4ty/repos","events_url":"https://api.github.com/users/prijutme4ty/events{/privacy}","received_events_url":"https://api.github.com/users/prijutme4ty/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/Mottie/tablesorter/labels/Enhancement","name":"Enhancement","color":"009800"},{"url":"https://api.github.com/repos/Mottie/tablesorter/labels/Widget","name":"Widget","color":"207de5"},{"url":"https://api.github.com/repos/Mottie/tablesorter/labels/Widget-Group","name":"Widget-Group","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-02-25T13:45:37Z","updated_at":"2015-08-01T09:03:11Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/Mottie/tablesorter/pulls/830","html_url":"https://github.com/Mottie/tablesorter/pull/830","diff_url":"https://github.com/Mottie/tablesorter/pull/830.diff","patch_url":"https://github.com/Mottie/tablesorter/pull/830.patch"},"body":"Hi!\r\nI've done some simplifications which will help in further refactoring. I'm going to introduce grouper objects so that all configuration to be done in constructor. It will prevent scattering grouping logic across entire widget codebase. Grouper object should have two methods: groupName and groupId (to be stored in `wo.group_currentGroup`). Also groupers are possible to be cached not to extract parameters each time (as it occurs now).\r\nActually I've done lots of steps in this direction, but I don't want publishing them until you check I didn't removed some unexpected undocumented functionality. I've listed all steps done in the list below.\r\n\r\nI hope it will also help me making hierarchical grouping (which is my final purpose, I need it for my work).\r\n\r\nHere there are simple changes, some of them just help in code organization, some remove unused options. What was done:\r\n1) Number grouping logic was too difficult. First it calculated an interval boundary and then it amended it by taking maximum of interval boundary and a different value. For ascending order that number was always less than a boundary, for descending order it was a very strange value... I can't figure out why this value was choosen. It came from the very first version of widget and may be it's just an old mistake which doesn't show itself for positive numbers.\r\nSo i just removed this amendment and now it's just an interval boundary. Now the code doesn't rely on group parameter, so it was removed.\r\nIf I'm wrong and this logic is necessary, please let me know, I will revert that change.\r\n\r\n2) Language parameter is unused and undocumented, so it was removed. If some grouping logic in future will be language-specific, I think it's better to pass it as an argument to a grouper constructor (I will show it in further commits with grouper object).\r\n\r\n3) Now number group name extractor extracts both boundaries. It changes a bit values to be compared dowstream (at line 111), but it doesn't change final result.\r\n\r\n4) Row is now obtained from a cache --> normalized --> $row.eq(0). (i.e. not-child row). I guess there are no situations when  `norm_rows[rowIndex]`  is falsy, so I replaced `currentGroup = norm_rows[rowIndex] ? <extract group name> : currentGroup` with `currentGroup = <extract group name>`. \r\nMay be I don't foresee some cases. Also, please let me know if it's true.\r\n\r\n5) Some complex calculations extracted into a separate variables and into a `groupHeaderHTML` function.\r\n\r\n6) `currentGroups` renamed into `collapsedGroups`. Initial value given in `bindEvent` changed from `[]` to `{}`.\r\n\r\n7) `ts.grouping` replaced by `this` because it's shorter and more specific.\r\n\r\nAs far as I see, these changes haven't broken anything. Widget works just as it did. When you have time, please, check that the changes listed make sense. I'm not sure they should be merged right now, because though it works good, it's still WIP.\r\np.s. Is it better to push further commits regarding grouping refactoring in the same pull request (so that once you're possible to merge them altogether) or to create separate pull requests?"},"comment":{"url":"https://api.github.com/repos/Mottie/tablesorter/issues/comments/126888929","html_url":"https://github.com/Mottie/tablesorter/pull/830#issuecomment-126888929","issue_url":"https://api.github.com/repos/Mottie/tablesorter/issues/830","id":126888929,"user":{"login":"prijutme4ty","id":814984,"avatar_url":"https://avatars.githubusercontent.com/u/814984?v=3","gravatar_id":"","url":"https://api.github.com/users/prijutme4ty","html_url":"https://github.com/prijutme4ty","followers_url":"https://api.github.com/users/prijutme4ty/followers","following_url":"https://api.github.com/users/prijutme4ty/following{/other_user}","gists_url":"https://api.github.com/users/prijutme4ty/gists{/gist_id}","starred_url":"https://api.github.com/users/prijutme4ty/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/prijutme4ty/subscriptions","organizations_url":"https://api.github.com/users/prijutme4ty/orgs","repos_url":"https://api.github.com/users/prijutme4ty/repos","events_url":"https://api.github.com/users/prijutme4ty/events{/privacy}","received_events_url":"https://api.github.com/users/prijutme4ty/received_events","type":"User","site_admin":false},"created_at":"2015-08-01T09:03:11Z","updated_at":"2015-08-01T09:03:11Z","body":"Hello, @Mottie. Sorry for a long delay.\r\nI'm not sure yet, about changes. Unfortunately I'm not able to take a thorough look on it till September. In a month I'll have much more relaxed work, and will return to tablesorter."}}