Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why aren't instances generated for all selects? #203

Closed
lognaturel opened this issue May 22, 2018 · 8 comments
Closed

Why aren't instances generated for all selects? #203

lognaturel opened this issue May 22, 2018 · 8 comments

Comments

@lognaturel
Copy link
Contributor

From survey.py:

Note that per commit message 0578242 and in xls2json.py R539, an
instance is only output for select items defined in the choices sheet
when the item has a choice_filter, and it is that way for backwards
compatibility.

From 0578242:

Switching back to old way of handling or_other selects, and no longer supporting them 
in filtered selects. Also, no longer using itemsets for selects that don't have 
a choice_filter. The reason for those changes is backwards compatibility.

Does anyone know which tool(s) this ensures compatibility with?

The commit was from 2012, is it possible it's not needed anymore? I'm asking because I like consistency and having less code to think about. I'm also thinking about additions/variations on instances and selects that we're introducing (e.g. #202, #176) and it would be simpler to only have one case with those.

CC @ukanga @MartijnR

@MartijnR
Copy link
Contributor

The advantage that the non-itemset selects have is that they can safely be considered to be static by a (crude) data collection client, and therefore don't risk adding a task to the evaluation cascade thereby improving form loading and form traversal performance.

However, I can see the appeal for simplifying code and a slightly intelligent data collection client would be able to discard itemset nodesets that do not have an XPath predicate (choice_filter) from the evaluation cascade.

So I'd have no problem to consistently use itemsets.

@lognaturel
Copy link
Contributor Author

Thanks, @MartijnR! Sorry, I'm not exactly sure what you mean by "evaluation cascade" in this context. I think of cascading in the context of recomputing dependent calculations using the XForms DAG when certain values change in the form. Do you mean that in the case of nodesets defined without an XPath predicate they may still be included in the DAG? I suppose in that case they wouldn't be dependent on anything, though, right? I think I'm missing something!

@MartijnR
Copy link
Contributor

MartijnR commented May 24, 2018

Yes, I meant all logic dependencies (CommCare calls it a cascade).

Indeed an itemset without a predicate refererring to a node inside the primary instance wouldn't be dependent on anything so shouldn't affect performance (and doesn't in Enketo). However, a less optimized client would just evaluate everything every time (like the first version of Enketo in 2012).

So I'm fine with this proposed change.

@lognaturel
Copy link
Contributor Author

Ok, that makes sense, thank you!

@ukanga any other possible guesses as to what backwards-compatibility might have been about?

I'm satisfied with having this conversation exist for future reference and to close the issue for now until there's more reason for the change. I don't think sometimes using static choice lists is blocking anything right now but I've wondered about why every time I've run into that code so thanks for clearing things up.

@lognaturel
Copy link
Contributor Author

Something that just came to mind and that would be worth verifying some time -- I don't think or_other works with itemsets currently. Though I think pyxform could inject an other item the same way it does with static items?

@craigappl
Copy link

Notes from breakout session:

Discussion

  • We generate a select, but when you have a choice filter, you create them without item sets.
  • The idea is to be able to create itemsets when we generate them, allowing us to query the filter elsewhere.
    (Need to continue...)

@tiritea
Copy link

tiritea commented Oct 14, 2019

My $0.02: itemsets are well-defined in the XForms specification; or_other is a hack, in lieu of supporting the equivalent - and arguably far better - XForm 'open selections'. I would be extremely reluctant to deprecate something that is officially spec'd, in favor of something that isnt [sic]

(I say "far better" because a properly implement open selection will add the user-defined value to the actual select-multi result, whereas with or_other you have to do some level of post-processing to generate a list with all the actual selected values (eg concat(${select}, " ", ${other}). This is needlessly messy...)

I do think its OK to always generate instances (instead of itemsets) when, for example, you absolutely need to additional expressiveness; specifically for translations.

@lognaturel
Copy link
Contributor Author

lognaturel commented Oct 16, 2019

@tiritea I believe your point about or_other is now captured at #218 (comment).

I thought it'd be nice to capture this conversation in some kind of dev doc/comment/something but I don't see a great place for it currently. Closing because there's no immediate action to take.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants