jhpyle / docassemble

A free, open-source expert system for guided interviews and document assembly, based on Python, YAML, and Markdown.
https://docassemble.org
MIT License
796 stars 258 forks source link

Non-unique IDs not caught at start of interview / warning area #215

Closed JCCChris closed 5 years ago

JCCChris commented 5 years ago

Currently, it's up to the developers to catch duplicated IDs. This is easy when projects are small and there are few questions, but when they are split up into multiple files and different developers are working on the project, it can be difficult to spot. This can result in idempotency errors at unexpected times and then a mental connection must be made between the duplicate ID and idempotency.

It could be an error to duplicate IDs if the idempotency setting is set to check.

jhpyle commented 5 years ago

I can see the benefit of that, but the problem is that it is valid to override an id. Suppose the file docassemble.a:data/questions/a.yml contains this:

id: fruit_first
supersedes: fruit_fallback
code: |
  if vegetable = 'turnip':
    fruit = 'avocado'
---
id: fruit_fallback
code: |
  fruit = 'apple'

Then another developer comes along and writes docassemble.b:data/questions/b.yml, which contains:

include:
  - docassemble.a:data/questions/a.yml
---
id: fruit_fallback
code: |
  fruit = 'banana'

In this situation, there would be two blocks with an id of fruit_fallback in the interview, but this is exactly what the developer wants. In the precedence ordering, the fruit_fallback block in package b will take the place of the fruit_fallback block in package a. When docassemble is seeking the definition of fruit, it will try the fruit_first block in package a and then try the fruit_fallback block in package b.

Note that you only need to use an id when writing mandatory blocks, when using supersedes, or non-field-setting buttons/choices. I would always almost always rather set a field than use the non-field-setting buttons/choices, so I don't encourage the use of these. If you are using ids for analytics, it is better to use ga id or segment id, which will not have any side effects. So there shouldn't be very many ids in your interview to begin with.

In any case, I'll add a warning to the console in the Playground if there are overridden ids.

jhpyle commented 5 years ago

Closing because the console warning discussed above was implemented.