{"action":"created","issue":{"url":"https://api.github.com/repos/testdouble/testdouble.js/issues/143","repository_url":"https://api.github.com/repos/testdouble/testdouble.js","labels_url":"https://api.github.com/repos/testdouble/testdouble.js/issues/143/labels{/name}","comments_url":"https://api.github.com/repos/testdouble/testdouble.js/issues/143/comments","events_url":"https://api.github.com/repos/testdouble/testdouble.js/issues/143/events","html_url":"https://github.com/testdouble/testdouble.js/issues/143","id":182552608,"number":143,"title":"Consider ignoreExtraArgs method instead of parameter","user":{"login":"focusaurus","id":482377,"avatar_url":"https://avatars.githubusercontent.com/u/482377?v=3","gravatar_id":"","url":"https://api.github.com/users/focusaurus","html_url":"https://github.com/focusaurus","followers_url":"https://api.github.com/users/focusaurus/followers","following_url":"https://api.github.com/users/focusaurus/following{/other_user}","gists_url":"https://api.github.com/users/focusaurus/gists{/gist_id}","starred_url":"https://api.github.com/users/focusaurus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/focusaurus/subscriptions","organizations_url":"https://api.github.com/users/focusaurus/orgs","repos_url":"https://api.github.com/users/focusaurus/repos","events_url":"https://api.github.com/users/focusaurus/events{/privacy}","received_events_url":"https://api.github.com/users/focusaurus/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-10-12T15:12:37Z","updated_at":"2016-10-14T16:55:36Z","closed_at":"2016-10-14T16:55:36Z","body":"I'd like to suggest making `ignoreExtraArgs` handled via a method call syntax instead of a function parameter to `td.when()`.\r\n\r\nSo instead of:\r\n\r\n```js\r\ntd.replace(db, 'query')\r\ntd.when(db.query('users', 'name'), {ignoreExtraArgs: true})\r\n  .thenResolve([])\r\n```\r\n\r\nI'd propose:\r\n\r\n```js\r\ntd.replace(db, 'query')\r\ntd.when(db.query('users', 'name'))\r\n  .ignoreExtraArgs()\r\n  .thenResolve([])\r\n```\r\n\r\nMy motivation is that after using testdouble heavily with a 5-developer team over 6 months, we've wasted a lot of time being confused and tracking down test failures when one of us makes the following very easy mistake:\r\n\r\n```js\r\n// What they intend/should code:\r\ntd.when(db.query('users', 'name'), {ignoreExtraArgs: true})\r\n  .thenResolve([])\r\n\r\n// What they accidentally code\r\ntd.when(db.query('users', 'name', {ignoreExtraArgs: true}))\r\n  .thenResolve([])\r\n```\r\n\r\nOne misplaced close parentheses sends them down a long debugging rabbit hole of confusion. This has happened several times to several developers, usually when refactoring existing code/tests. We know to watch for it now, but it was a hard-learned lesson.\r\n\r\nThe tests typically end up throwing an exception `TypeError: Cannot read property 'then' of undefined`, which in a promise-heavy codebase is tantamount to \"something didn't return a promise when it should have\", but that implicates almost every function in the project.\r\n\r\nI believe the explicit method syntax will reduce mistakes here, especially when formatted as above with each chained dot operator starting a new line.\r\n\r\nI have only briefly looked at the source of `src/when.coffee` and I believe this changed API to be technically possible, but I could be wrong about that.\r\n"},"comment":{"url":"https://api.github.com/repos/testdouble/testdouble.js/issues/comments/253858466","html_url":"https://github.com/testdouble/testdouble.js/issues/143#issuecomment-253858466","issue_url":"https://api.github.com/repos/testdouble/testdouble.js/issues/143","id":253858466,"user":{"login":"searls","id":79303,"avatar_url":"https://avatars.githubusercontent.com/u/79303?v=3","gravatar_id":"","url":"https://api.github.com/users/searls","html_url":"https://github.com/searls","followers_url":"https://api.github.com/users/searls/followers","following_url":"https://api.github.com/users/searls/following{/other_user}","gists_url":"https://api.github.com/users/searls/gists{/gist_id}","starred_url":"https://api.github.com/users/searls/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/searls/subscriptions","organizations_url":"https://api.github.com/users/searls/orgs","repos_url":"https://api.github.com/users/searls/repos","events_url":"https://api.github.com/users/searls/events{/privacy}","received_events_url":"https://api.github.com/users/searls/received_events","type":"User","site_admin":false},"created_at":"2016-10-14T16:55:36Z","updated_at":"2016-10-14T16:55:36Z","body":"Thanks for your proposal and for providing an (entirely valid and understandable!) rationale, however I'm not willing to move `td.when` and `td.verify` to a chaining API. A few reasons:\r\n\r\n1. `td.when` currently returns the test double, which itself is necessary for one-lineable instantiation. (example: `td.when(td.function()('pants')).thenReturn('yup')` return the double that returns `'yup'`. Moving to a chaining API would prevent this because we'd instead have to return some chainable-object (oddly enough, it was [exactly this problem](https://github.com/sinonjs/sinon/issues/817) I raised on Sinon that led me to write testdouble.js).\r\n\r\n2. You could (and should) point out at this point that chained _configuration_ methods don't resolve the chain so those are okay, and that `thenReturn` and so on can safely do so. You'd be right, but that's a lot of nuance for users to keep in their head. When people see a chaining/fluent API they generally assume it's safe to call whatever they like in whatever order they like, and we'd have to educate them about _why this config is different_ (and I'm not sure it is).\r\n\r\n3. If you read the aforementioned sinon [issue](https://github.com/sinonjs/sinon/issues/817) you'll see mention of lots of issues being opened because of the move to a chainable API. these things are really hard to test and manage, and are much trickier to implement (and harder to understand) than a single-depth configuration object\r\n\r\n4. It's my opinion that the vast majority of `when` and `verify` calls should not require matchers nor custom configuration options. If you're finding that most or all of your stubbings are also passing `ignoreExtraArgs`, then it's my opinion that those tests are incomplete because they're not verifying the args are what you expect, and so I wouldn't want to change the library to encourage frequent use of those options. They exist because there are cases when they're genuinely necessary, but every time you reach for any of them it should feel a little painful as if to remind you that _always_ leaning on them is an anti-pattern.\r\n\r\nIf you disagree with the above or see things differently, I'd rather dig into the root cause issue of why you're always setting `ignoreExtraArgs`, rather than guard against typos when setting it so often (which seems to me like treating the symptom). Moreover, if we just see things differently, I'd recommend you write a test helper that encapsulates this pattern for yourself so that you're not calling `td.when` directly but instead setting things up how you always do and without the risk for error or the needless verbosity from repeating yourself."}}
{"actor":{"display_login":"searls"}}