{"action":"created","issue":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/173","labels_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/173/labels{/name}","comments_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/173/comments","events_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/173/events","html_url":"https://github.com/dimitar-asenov/Envision/pull/173","id":111676964,"number":173,"title":"Optional returnvalue","user":{"login":"lukedirtwalker","id":3530769,"avatar_url":"https://avatars.githubusercontent.com/u/3530769?v=3","gravatar_id":"","url":"https://api.github.com/users/lukedirtwalker","html_url":"https://github.com/lukedirtwalker","followers_url":"https://api.github.com/users/lukedirtwalker/followers","following_url":"https://api.github.com/users/lukedirtwalker/following{/other_user}","gists_url":"https://api.github.com/users/lukedirtwalker/gists{/gist_id}","starred_url":"https://api.github.com/users/lukedirtwalker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukedirtwalker/subscriptions","organizations_url":"https://api.github.com/users/lukedirtwalker/orgs","repos_url":"https://api.github.com/users/lukedirtwalker/repos","events_url":"https://api.github.com/users/lukedirtwalker/events{/privacy}","received_events_url":"https://api.github.com/users/lukedirtwalker/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-10-15T18:20:32Z","updated_at":"2015-10-19T12:13:37Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/173","html_url":"https://github.com/dimitar-asenov/Envision/pull/173","diff_url":"https://github.com/dimitar-asenov/Envision/pull/173.diff","patch_url":"https://github.com/dimitar-asenov/Envision/pull/173.patch"},"body":"Okay here the new proposal for runtime error handling. It is not complete yet so please do not merge, but please let me know if it is okayish?\r\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227455%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227568%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227636%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227685%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42232014%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42232152%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42234677%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42234795%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235122%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235179%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235384%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235421%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235475%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235632%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23issuecomment-148701086%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235953%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42237866%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42237970%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42238460%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42238484%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42241156%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42360969%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/queries/TagQuery.cpp%2016%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235475%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Do%20we%20not%20want%20this%20alias%20any%20more%2C%20or%20why%20did%20this%20get%20commented%3F%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A08%3A35Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22That%27s%20just%20an%20oversight.%20It%20should%20still%20be%20here.%20Thanks%20for%20noticing%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A47%3A08Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/queries/TagQuery.cpp%3AL56-65%22%7D%2C%20%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/misc/Optional.h%2066%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42234795%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Hmm%2C%20The%20optional%20type%20needs%20to%20support%20not%20one%2C%20but%20many%20warnings.%20Imagine%20that%20during%20execution%2C%20multiple%20queries%20give%20a%20warning%2C%20and%20then%20finally%20we%20get%20an%20error%20from%20somewhere.%20We%20want%20to%20show%20all%20of%20these%20to%20the%20user.%22%2C%20%22created_at%22%3A%20%222015-10-16T11%3A58%3A08Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22So%20you%20suggest%20to%20have%20a%20collection%20of%20warnings%20in%20here%3F%20This%20starts%20to%20feel%20kind%20of%20heavy%20to%20me.%20But%20on%20the%20other%20hand%20I%20don%27t%20see%20a%20better%20solution%20a%20the%20moment.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A45%3A40Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%2C%20%7B%22body%22%3A%20%22More%20abstractly%2C%20we%20need%20a%20way%20to%20describe%20why%20an%20optional%20is%20empty.%20In%20our%20particular%20case%2C%20this%20could%20be%20due%20to%20one%20or%20several%20errors%2C%20which%20were%20possibly%20preceded%20by%20one%20or%20several%20warnings.%20I%20agree%20it%27s%20somewhat%20heavy%20here%2C%20but%20on%20the%20bright%20side%2C%20the%20code%20where%20you%20create/handle%20the%20warnings/errors%2C%20doesn%27t%20change%20much.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A52%3A51Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/misc/Optional.h%3AL1-123%22%7D%2C%20%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/queries/UnionOperator.cpp%208%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235632%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Hmm%2C%20now%20that%20I%20look%20at%20this%20example%2C%20I%27m%20not%20sure%20if%20we%20should%20have%20%60QList%3COptional%3CTupleSet%3E%3E%60%20or%20%60Optional%3CQList%3CTupleSet%3E%3E%60.%20What%20are%20pros%20and%20cons%20of%20each%20approach%3F%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A11%3A01Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/queries/UnionOperator.cpp%3AL28-37%22%7D%2C%20%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/queries/CompositeQuery.cpp%2023%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235122%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22So%20in%20places%20like%20this.%20You%20shouldn%27t%20ignore%20other%20potential%20errors%2C%20warnings%2C%20but%20you%20should%20combine%20them%20all%20and%20output%20all.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A02%3A49Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22You%20should%20probably%20implement%20a%20merge%20funtion%20inside%20Optional%2C%20which%20takes%20a%20list%20of%20Optional%20and%20gives%20you%20back%20one%20Optional%2C%20which%20you%20can%20finally%20check%20for%20error%20and%20break%20if%20it%20has%20at%20least%20one%20error.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A03%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22Hmm%2C%20now%20that%20I%20think%20about%20it%2C%20I%27m%20not%20sure%20about%20the%20merge%20function%20and%20where%20it%20should%20be.%20We%20want%20to%20transition%20from%20a%20%60QList%3COptional%3E%60%20to%20%60Optional%3CQList%3C%60.%20See%20also%20some%20of%20my%20comments%20below%20about%20this.%20Perhaps%2C%20logically%2C%20the%20correct%20thing%20to%20have%20is%20indeed%20%60Optional%3CQList%3CTupleSet%3E%3E%60%20as%20the%20return%20type%20of%20%60Query%3A%3Aexecute%60.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A16%3A44Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22About%20the%20first%20comment%2C%20in%20that%20place%20there%20should%20only%20ever%20be%20one%20error%20%28otherwise%20we%20would%20have%20aborted%20already%29%2C%20but%20maybe%20multiple%20warnings.%5Cr%5Cn%5Cr%5CnI%20will%20have%20to%20think%20about%20Optional%3CQList%20vs%20QList%3COptional%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A52%3A34Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%2C%20%7B%22body%22%3A%20%22Well%2C%20what%20about%20a%20hypothetical%20scenario%20where%20two%20independent%20branches%20of%20a%20composite%20query%20both%20have%20an%20error.%20Should%20we%20just%20pick%20one%20of%20those%20at%20random%20and%20return%20it%3F%20Especially%20later%2C%20if%20we%20allow%20parallel%20execution%20at%20some%20point%2C%20this%20will%20be%20a%20problem.%5Cr%5Cn%5Cr%5CnI%20guess%20if%20you%20convert%20everything%20to%20%60Optional%3CQList%3C..%3E%3E%60%20then%20in%20the%20sequential%20case%2C%20we%20can%20still%20get%20away%20with%20one%20error.%5Cr%5Cn%5Cr%5CnMight%20be%20nicer/easier%20though%20to%20treat%20errors%20and%20warnings%20almost%20identically%20in%20%60Optional%60%2C%20so%20to%20have%20a%20list%20that%20can%20hold%20either.%22%2C%20%22created_at%22%3A%20%222015-10-16T13%3A22%3A43Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22If%20we%20allow%20parallel%20execution%20we%20will%20have%20to%20rewrite%20this%20code%20anyway.%20Therefore%20I%20will%20simply%20return%20the%20error%20Optional%20if%20there%20is%20an%20error.%5Cr%5Cn%5Cr%5CnI%27m%20not%20sure%20why%20QList%3COptional%3C..%20would%20hinder%20me%20from%20doing%20this%3F%22%2C%20%22created_at%22%3A%20%222015-10-19T11%3A37%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/queries/CompositeQuery.cpp%3AL77-90%22%7D%2C%20%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/queries/ScriptQuery.cpp%2019%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235421%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%2C%20always%20use%20%60using%20...%20%3D%20...%60%20instead%20of%20%60typedef%60.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A07%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/queries/ScriptQuery.cpp%3AL97-104%22%7D%2C%20%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23issuecomment-148701086%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Overall%2C%20I%20think%20I%20like%20the%20error%20handling%20direction.%20We%20just%20need%20to%20figure%20out%20how%20to%20handle%20lists%20of%20Optionals.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A14%3A00Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%204a5fe62fb15575c3453c8bda466b5bbf8f6620ca%20InformationScripting/src/queries/QueryExecutor.cpp%2014%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42235384%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22We%20should%20return%20this%20to%20the%20command%20prompt%20and%20display%20it%20inside%20the%20command%20result.%22%2C%20%22created_at%22%3A%20%222015-10-16T12%3A07%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/queries/QueryExecutor.cpp%3AL44-60%22%7D%2C%20%22Pull%20c76e78c25018c6625fd6c2e902d855256ba417d4%20InformationScripting/src/misc/Optional.h%2042%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227636%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22can%20%60QString%60%20ever%20be%20%60constrexp%60%20%3F%20I%20don%27t%20think%20so%20as%20it%20has%20heap%20allocation.%20Therefore%2C%20I%20think%20these%20methods%20can%20never%20be%20used%20as%20%60constexpr%60%20so%20you%20should%20remove%20that.%22%2C%20%22created_at%22%3A%20%222015-10-16T10%3A11%3A08Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20same%20applies%20for%20all%20the%20other%20methods%20I%20believe.%22%2C%20%22created_at%22%3A%20%222015-10-16T10%3A11%3A47Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22True%22%2C%20%22created_at%22%3A%20%222015-10-16T11%3A18%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/misc/Optional.h%3AL1-123%22%7D%2C%20%22Pull%20c76e78c25018c6625fd6c2e902d855256ba417d4%20InformationScripting/src/misc/Optional.h%2040%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/173%23discussion_r42227455%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Hmm%2C%20I%20didn%27t%20know%20that%20one%20can%20put%20a%20static%20assert%20directly%20like%20this.%20We%20have%20no%20good%20way%20of%20expressing%20this%20in%20Envision%20at%20the%20moment.%20Good%20to%20know%20%3A%29%22%2C%20%22created_at%22%3A%20%222015-10-16T10%3A08%3A29Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22But%20do%20we%20actually%20need%20this%3F%20If%20you%20remove%20the%20%60static_assert%60%20compilation%20will%20still%20fail%20statically%2C%20as%20the%20constructors%20below%20will%20be%20ambiguous%2C%20or%20am%20I%20wrong%3F%22%2C%20%22created_at%22%3A%20%222015-10-16T10%3A10%3A10Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22True%2C%20but%20the%20message%20would%20be%20more%20clear%20than%20a%20list%20of%20errors%20about%20ambigious%20contrsuctors.%20I%20can%20remove%20it%20if%20you%20want.%22%2C%20%22created_at%22%3A%20%222015-10-16T11%3A20%3A33Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%2C%20%7B%22body%22%3A%20%22Let%27s%20leave%20it%20for%20now.%22%2C%20%22created_at%22%3A%20%222015-10-16T11%3A56%3A32Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20InformationScripting/src/misc/Optional.h%3AL1-123%22%7D%7D%7D'></a>\r\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\r\n- [x] <a href='#crh-comment-Pull c76e78c25018c6625fd6c2e902d855256ba417d4 InformationScripting/src/misc/Optional.h 40'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42227455'>File: InformationScripting/src/misc/Optional.h:L1-123</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Hmm, I didn't know that one can put a static assert directly like this. We have no good way of expressing this in Envision at the moment. Good to know :)\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> But do we actually need this? If you remove the `static_assert` compilation will still fail statically, as the constructors below will be ambiguous, or am I wrong?\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> True, but the message would be more clear than a list of errors about ambigious contrsuctors. I can remove it if you want.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Let's leave it for now.\r\n- [x] <a href='#crh-comment-Pull c76e78c25018c6625fd6c2e902d855256ba417d4 InformationScripting/src/misc/Optional.h 42'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42227636'>File: InformationScripting/src/misc/Optional.h:L1-123</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> can `QString` ever be `constrexp` ? I don't think so as it has heap allocation. Therefore, I think these methods can never be used as `constexpr` so you should remove that.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> The same applies for all the other methods I believe.\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> True\r\n- [x] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/misc/Optional.h 66'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42234795'>File: InformationScripting/src/misc/Optional.h:L1-123</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Hmm, The optional type needs to support not one, but many warnings. Imagine that during execution, multiple queries give a warning, and then finally we get an error from somewhere. We want to show all of these to the user.\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> So you suggest to have a collection of warnings in here? This starts to feel kind of heavy to me. But on the other hand I don't see a better solution a the moment.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> More abstractly, we need a way to describe why an optional is empty. In our particular case, this could be due to one or several errors, which were possibly preceded by one or several warnings. I agree it's somewhat heavy here, but on the bright side, the code where you create/handle the warnings/errors, doesn't change much.\r\n- [x] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/queries/CompositeQuery.cpp 23'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42235122'>File: InformationScripting/src/queries/CompositeQuery.cpp:L77-90</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> So in places like this. You shouldn't ignore other potential errors, warnings, but you should combine them all and output all.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> You should probably implement a merge funtion inside Optional, which takes a list of Optional and gives you back one Optional, which you can finally check for error and break if it has at least one error.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Hmm, now that I think about it, I'm not sure about the merge function and where it should be. We want to transition from a `QList<Optional>` to `Optional<QList<`. See also some of my comments below about this. Perhaps, logically, the correct thing to have is indeed `Optional<QList<TupleSet>>` as the return type of `Query::execute`.\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> About the first comment, in that place there should only ever be one error (otherwise we would have aborted already), but maybe multiple warnings.\r\nI will have to think about Optional<QList vs QList<Optional\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Well, what about a hypothetical scenario where two independent branches of a composite query both have an error. Should we just pick one of those at random and return it? Especially later, if we allow parallel execution at some point, this will be a problem.\r\nI guess if you convert everything to `Optional<QList<..>>` then in the sequential case, we can still get away with one error.\r\nMight be nicer/easier though to treat errors and warnings almost identically in `Optional`, so to have a list that can hold either.\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> If we allow parallel execution we will have to rewrite this code anyway. Therefore I will simply return the error Optional if there is an error.\r\nI'm not sure why QList<Optional<.. would hinder me from doing this?\r\n- [x] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/queries/QueryExecutor.cpp 14'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42235384'>File: InformationScripting/src/queries/QueryExecutor.cpp:L44-60</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> We should return this to the command prompt and display it inside the command result.\r\n- [x] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/queries/ScriptQuery.cpp 19'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42235421'>File: InformationScripting/src/queries/ScriptQuery.cpp:L97-104</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Please, always use `using ... = ...` instead of `typedef`.\r\n- [x] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/queries/TagQuery.cpp 16'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42235475'>File: InformationScripting/src/queries/TagQuery.cpp:L56-65</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Do we not want this alias any more, or why did this get commented?\r\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> That's just an oversight. It should still be here. Thanks for noticing\r\n- [ ] <a href='#crh-comment-Pull 4a5fe62fb15575c3453c8bda466b5bbf8f6620ca InformationScripting/src/queries/UnionOperator.cpp 8'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#discussion_r42235632'>File: InformationScripting/src/queries/UnionOperator.cpp:L28-37</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Hmm, now that I look at this example, I'm not sure if we should have `QList<Optional<TupleSet>>` or `Optional<QList<TupleSet>>`. What are pros and cons of each approach?\r\n- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/173#issuecomment-148701086'>General Comment</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Overall, I think I like the error handling direction. We just need to figure out how to handle lists of Optionals.\r\n\r\n\r\n<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/173?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/173?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/dimitar-asenov/Envision/pull/173'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\r\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/comments/149197374","html_url":"https://github.com/dimitar-asenov/Envision/pull/173#issuecomment-149197374","issue_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/173","id":149197374,"user":{"login":"lukedirtwalker","id":3530769,"avatar_url":"https://avatars.githubusercontent.com/u/3530769?v=3","gravatar_id":"","url":"https://api.github.com/users/lukedirtwalker","html_url":"https://github.com/lukedirtwalker","followers_url":"https://api.github.com/users/lukedirtwalker/followers","following_url":"https://api.github.com/users/lukedirtwalker/following{/other_user}","gists_url":"https://api.github.com/users/lukedirtwalker/gists{/gist_id}","starred_url":"https://api.github.com/users/lukedirtwalker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukedirtwalker/subscriptions","organizations_url":"https://api.github.com/users/lukedirtwalker/orgs","repos_url":"https://api.github.com/users/lukedirtwalker/repos","events_url":"https://api.github.com/users/lukedirtwalker/events{/privacy}","received_events_url":"https://api.github.com/users/lukedirtwalker/received_events","type":"User","site_admin":false},"created_at":"2015-10-19T12:13:36Z","updated_at":"2015-10-19T12:13:36Z","body":"So I think this is now more or less ready. Should I rebase or merge upstream once you think it's ready? I think rebase would be nicer."}}