{"action":"created","issue":{"url":"https://api.github.com/repos/bali182/autocomplete-json/issues/40","repository_url":"https://api.github.com/repos/bali182/autocomplete-json","labels_url":"https://api.github.com/repos/bali182/autocomplete-json/issues/40/labels{/name}","comments_url":"https://api.github.com/repos/bali182/autocomplete-json/issues/40/comments","events_url":"https://api.github.com/repos/bali182/autocomplete-json/issues/40/events","html_url":"https://github.com/bali182/autocomplete-json/issues/40","id":140427716,"number":40,"title":"Schema Visitor pattern feedback","user":{"login":"bd82","id":4233843,"avatar_url":"https://avatars.githubusercontent.com/u/4233843?v=3","gravatar_id":"","url":"https://api.github.com/users/bd82","html_url":"https://github.com/bd82","followers_url":"https://api.github.com/users/bd82/followers","following_url":"https://api.github.com/users/bd82/following{/other_user}","gists_url":"https://api.github.com/users/bd82/gists{/gist_id}","starred_url":"https://api.github.com/users/bd82/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bd82/subscriptions","organizations_url":"https://api.github.com/users/bd82/orgs","repos_url":"https://api.github.com/users/bd82/repos","events_url":"https://api.github.com/users/bd82/events{/privacy}","received_events_url":"https://api.github.com/users/bd82/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2016-03-12T23:34:09Z","updated_at":"2016-03-16T10:07:49Z","closed_at":null,"body":"Hi @bali182.\r\n\r\nContinuing our discussion from https://github.com/SAP/chevrotain/issues/135.\r\n\r\nI've looked at https://github.com/bali182/autocomplete-json/blob/master/lib/json-schema-visitors.ts\r\n\r\nThe implementation looks clean and clear. There are a few issues I can raise:\r\n\r\n**Major issue:**\r\nIt seems like the visitors handle the concern of  traversing the data structure.\r\n```Typescript\r\nvisitAnyOfSchema(schema: AnyOfSchema, collector: Array<BaseSchema>): void {\r\n    schema.getSchemas().forEach(childSchema => childSchema.accept(this, collector));\r\n  }\r\n```\r\n\r\nThis means the visitors need to be aware of internal details of the data structure and not just\r\nperforming dispatching by type.\r\nImagine adding another layer of nesting (composition) in your data structure.\r\nNow **all** the visitors implementations will have to **modified** to accept that nested property as well.\r\n```Typescript\r\n// ALL visitAnyOfSchema will have to modified in ALL visitors if we add a new property that needs be visited.\r\nvisitAnyOfSchema(schema: AnyOfSchema, collector: Array<BaseSchema>): void {\r\n    schema.getSchemas().forEach(childSchema => childSchema.accept(this, collector));\r\n    schema.getNewNestedProp().accept(this, collector);  // this change will appear in many visitors\r\n  }\r\n```\r\n\r\nA solution for this would be for the accept method on the classes in the data structure to deal with the children.\r\n\r\n```Typescript\r\n  accept<P, R>(visitor: ISchemaVisitor<P, R>, parameter: P): R[] {\r\n    let myResult = visitor.visitAnyOfSchema(this, parameter);\r\n    let childrenResults = this.getSchemas().map( (currChild) => currChild.accept(this, parameter);\r\n    return [myResult].concat(childrenResults);\r\n  }\r\n```\r\n\r\nThis means changes in the data structure won't require changes to the visitors.\r\nThe visitors only need to handle the dispatching.\r\n\r\n "},"comment":{"url":"https://api.github.com/repos/bali182/autocomplete-json/issues/comments/197242544","html_url":"https://github.com/bali182/autocomplete-json/issues/40#issuecomment-197242544","issue_url":"https://api.github.com/repos/bali182/autocomplete-json/issues/40","id":197242544,"user":{"login":"bali182","id":3879181,"avatar_url":"https://avatars.githubusercontent.com/u/3879181?v=3","gravatar_id":"","url":"https://api.github.com/users/bali182","html_url":"https://github.com/bali182","followers_url":"https://api.github.com/users/bali182/followers","following_url":"https://api.github.com/users/bali182/following{/other_user}","gists_url":"https://api.github.com/users/bali182/gists{/gist_id}","starred_url":"https://api.github.com/users/bali182/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bali182/subscriptions","organizations_url":"https://api.github.com/users/bali182/orgs","repos_url":"https://api.github.com/users/bali182/repos","events_url":"https://api.github.com/users/bali182/events{/privacy}","received_events_url":"https://api.github.com/users/bali182/received_events","type":"User","site_admin":false},"created_at":"2016-03-16T10:07:49Z","updated_at":"2016-03-16T10:07:49Z","body":"Hmm... Thanks for bringing these up. The thing is, I tried to convince you by showing you these examples, that having leaf-class visits is the way to go, but I see your point :smile: . Sure, it would be less code if I had a `visitCompositeSchema()` method in the interface, but that would introduce some ambiguity. Where do I implement accept? In the base class? In each sub classes? If i do it in the base class, how do I know, that the visitor wants to handle all the leaf classes inheriting from the abstract base (`CompositeSchema`) or it just wants to catch all? Maybe do I dispatch to both `visitCompositeSchema()` and `visitXySubtype()` ?\r\n\r\nWhat you noticed about having to change the visitors when something changes in the class hierarchy it visits, was very conscious decision. I would like the compiler to notify me, when I add for example a new `CompositeSchema` subclass, so even if the class hierarchy gets bigger, I'd have to make a conscious decision about how to handle each new class in each visitor.\r\n\r\nI guess it's just a difference in how we look at things, I was mostly using statically typed languages, so js is kinda new to me, so maybe I'm trying to force it to do too many things that makes no sense from js perspective.\r\n\r\nI looked at the visitor you linked in more detail. It seems like a \"reflection\" based solution, but it seems very reasonable if you don't know exactly what you will be visiting, and you want something dynamically exploring and calling methods for types in any arbitrary class hierarchy. Also the error which it is thrown in case of a type not handled, makes it as safe as sanely possible. It seems like you also have the problem tackled that I mentioned in the first paragraph (about validating ambiguity). Maybe it's not a problem that the type system should handle? Anyways, I like this solution."}}