{"action":"created","issue":{"url":"https://api.github.com/repos/envato/double_entry/issues/99","labels_url":"https://api.github.com/repos/envato/double_entry/issues/99/labels{/name}","comments_url":"https://api.github.com/repos/envato/double_entry/issues/99/comments","events_url":"https://api.github.com/repos/envato/double_entry/issues/99/events","html_url":"https://github.com/envato/double_entry/pull/99","id":100733071,"number":99,"title":"Fix calling of scope lambda with the result of said lambda, rather than expected input ( WIP, ready for comments )","user":{"login":"eadz","id":11939,"avatar_url":"https://avatars.githubusercontent.com/u/11939?v=3","gravatar_id":"","url":"https://api.github.com/users/eadz","html_url":"https://github.com/eadz","followers_url":"https://api.github.com/users/eadz/followers","following_url":"https://api.github.com/users/eadz/following{/other_user}","gists_url":"https://api.github.com/users/eadz/gists{/gist_id}","starred_url":"https://api.github.com/users/eadz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eadz/subscriptions","organizations_url":"https://api.github.com/users/eadz/orgs","repos_url":"https://api.github.com/users/eadz/repos","events_url":"https://api.github.com/users/eadz/events{/privacy}","received_events_url":"https://api.github.com/users/eadz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":7,"created_at":"2015-08-13T10:23:31Z","updated_at":"2016-02-01T14:36:27Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/envato/double_entry/pulls/99","html_url":"https://github.com/envato/double_entry/pull/99","diff_url":"https://github.com/envato/double_entry/pull/99.diff","patch_url":"https://github.com/envato/double_entry/pull/99.patch"},"body":"TLDR;\r\n\r\nThe *current* API for a scope lambda is something like:\r\n\r\n`A`) The lambda should take an object ( which is passed as `scope: object` ) and return a string representation of that object to be saved in the database as `scope`\r\nAND\r\n`B`) The lambda should take a string previously returned by itself as input, and return that string again\r\n\r\nThis PR removes requirement `B`, (because why call the lambda more than once, and with its own output as input?)\r\n\r\n...\r\n\r\n\r\n# Problem \r\n\r\nWhen you are using a lambda scope, e.g. \r\n\r\n```ruby\r\n-> (user) { user.id }\r\n```\r\nobviously you expect a User object, however it is called with a string in some cases. I believe this used to not happen.\r\n\r\nThe cause of this is due to instantiation of an account object from the account balance class, where it just has the string, but passes the string as :scope to account.\r\n\r\nHere the scope identifier is called with the :scope param ( e.g. a User )\r\n\r\n```ruby\r\nmodule DoubleEntry\r\n  class Account\r\n    class Instance\r\n      # ...\r\n      def initialize(args)\r\n        @account = args[:account]\r\n        @scope = args[:scope]\r\n        ensure_scope_is_valid\r\n      end\r\n\r\n      def scope_identity\r\n        scope_identifier.call(scope).to_s if scoped?\r\n      end\r\n```\r\n\r\nbut here, it's passed in as a string:\r\n\r\n```ruby\r\n  class AccountBalance < ActiveRecord::Base\r\n    delegate :currency, :to => :account\r\n\r\n   # ...\r\n    def account=(account)\r\n      self[:account] = account.identifier.to_s\r\n      self[:scope] = account.scope_identity # scope identity is the result of the scope call\r\n      account\r\n    end\r\n\r\n    def account\r\n      DoubleEntry.account(self[:account].to_sym, :scope => self[:scope]) # but here it's passed in like a User object would be\r\n    end\r\n ```\r\n\r\nThe AR Scope factory works around this by checking for a string, but it seems hacky to need to do this for all custom scopes. It is basically circular logic with the scope lambda output being put back into the scope lambda as input, for no good reason. This causes problems when you're not using the ARScopeFactory. \r\n\r\n# Solution 1?\r\n\r\nThis change adds another way of instantiating an account object. In addition to `:scope`, you can also pass in `:scope_identity` which is the pre-computed output of the scope lambda. \r\n\r\nThis PR is not 100% finished, I think there may need to be some documentation changes and some more tests, but I'm looking for feedback before I continue much further.\r\n\r\n# Solution 2?\r\n\r\nA non-backwards compatible solution going forward may be to remove the idea of a scope lambda all-together. Just pass in the string or number identifier you want stored in the database instead. You could still use a scope lambda to generate this in the client application, but separate out the responsibility of going from `User#id=22` to `22` from DoubleEntry itself. I think this would simplify the code quite a lot, for not much of a loss in functionality and the benefit of less 'magic'.\r\n"},"comment":{"url":"https://api.github.com/repos/envato/double_entry/issues/comments/177996900","html_url":"https://github.com/envato/double_entry/pull/99#issuecomment-177996900","issue_url":"https://api.github.com/repos/envato/double_entry/issues/99","id":177996900,"user":{"login":"eadz","id":11939,"avatar_url":"https://avatars.githubusercontent.com/u/11939?v=3","gravatar_id":"","url":"https://api.github.com/users/eadz","html_url":"https://github.com/eadz","followers_url":"https://api.github.com/users/eadz/followers","following_url":"https://api.github.com/users/eadz/following{/other_user}","gists_url":"https://api.github.com/users/eadz/gists{/gist_id}","starred_url":"https://api.github.com/users/eadz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eadz/subscriptions","organizations_url":"https://api.github.com/users/eadz/orgs","repos_url":"https://api.github.com/users/eadz/repos","events_url":"https://api.github.com/users/eadz/events{/privacy}","received_events_url":"https://api.github.com/users/eadz/received_events","type":"User","site_admin":false},"created_at":"2016-02-01T14:36:27Z","updated_at":"2016-02-01T14:36:27Z","body":"I'm a little disappointed it took 5 months for this PR to get a look at TBH, and now there are merge conflicts to resolve. In that time, I have changed jobs and I now no longer use double entry at work, so I don't expect my involvement in this project to be much going forward mainly because I don't use it at work anymore. \r\n\r\nI think that this change is a little hairy as it changes some core parts, and I would recommend that it gets a really solid look at before merging, and maybe resolving the merge conflicts can be part of that process. Sorry to leave it like this but I hope you can understand I'm no longer in the double entry headspace! "}}