Headers Override for Name and CrowdAnkiUUID

[ohare93]

Added the ability to override name and crowdanki_uuid for a Deck Header.

For anki-geo/ultimate-geography#566

[aplaice]

Thanks so much for the fast implementation! It works! :)

Isn't a (slight) design issue that this prevents BrainBrew from being able to (even in principle) import changes to the deck name (and the crowdanki_uuid)? (when running anki_to_source)

(BrainBrew currently doesn't allow writing to the YAML headers file, but in principle it's possible, while if the name and crowdanki_uuid are hard-coded in the recipes then it becomes impossible even in principle.)

(I'm probably heavily overthinking this, though — anki_to_source is currently probably only rarely used, and changing the deck name (and especially the deck UUID) from the Anki side will be extremely rare.)

---

Edit: One tiny thing I noticed is that in the built JSONs, the crowdanki_uuid key is now after the desc key (while all the other keys are still sorted).

[aplaice]

I think that we need default values (None) here, if we want to allow overriding only some of deck_description, crowdanki_uuid and name at the same time.

Suggested change

	deck_description_html_file: Optional[str]
	crowdanki_uuid: Optional[str]
	name: Optional[str]
	deck_description_html_file: Optional[str] = None
	crowdanki_uuid: Optional[str] = None
	name: Optional[str] = None

[ohare93]

I don't believe so, since the actual overriding function individually check if each value is truthy. Have you experienced that populating one in the recipe, and not others, will null out the others?

[aplaice]

Overriding one in the recipe, but not the others, causes BrainBrew to crash:

TypeError: __init__() missing 2 required positional arguments: 'crowdanki_uuid' and 'name'

Full backtrace

INFO:root:Builder file recipes/source_to_anki.yaml is ✔ good
INFO:root:Attempting to generate Guids
INFO:root:Generate guids complete
Traceback (most recent call last):
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/bin/brain_brew", line 33, in <module>
    sys.exit(load_entry_point('Brain-Brew', 'console_scripts', 'brain_brew')())
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/main.py", line 19, in main
    command.execute()
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/run_recipe.py", line 15, in execute
    recipe = TopLevelBuilder.parse_and_read(self.recipe_file_name, self.verify_only)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/top_level_builder.py", line 60, in parse_and_read
    return cls.from_list(recipe_data)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/recipe_builder.py", line 20, in from_list
    tasks = cls.read_tasks(data)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/recipe_builder.py", line 68, in read_tasks
    task_or_tasks = [matching_task.from_repr(task_arguments)]
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/parts_builder.py", line 40, in from_repr
    return cls.from_list(data)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/recipe_builder.py", line 20, in from_list
    tasks = cls.read_tasks(data)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/recipe_builder.py", line 66, in read_tasks
    task_or_tasks = [matching_task.from_repr(t_arg) for t_arg in task_arguments]
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/commands/run_recipe/recipe_builder.py", line 66, in <listcomp>
    task_or_tasks = [matching_task.from_repr(t_arg) for t_arg in task_arguments]
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/build_tasks/deck_parts/headers_from_yaml_part.py", line 58, in from_repr
    override=HeadersOverride.from_repr(rep.override) if rep.override else None
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/build_tasks/overrides/headers_override.py", line 32, in from_repr
    rep: cls.Representation = data if isinstance(data, cls.Representation) else cls.Representation.from_dict(data)
  File "/home/adam/.local/share/virtualenvs/git-1CgJDxfG/src/brain-brew/brain_brew/configuration/representation_base.py", line 18, in from_dict
    return cls(**expected_values)
TypeError: __init__() missing 2 required positional arguments: 'crowdanki_uuid' and 'name'

[ohare93]

Huh, I guess you're right. I never made that default value in the original since it was the only member of the Override class. Pushed a change that should fix it 👍

[ohare93]

Thanks so much for the fast implementation! It works! :)

No worries, any time 😁

Isn't a (slight) design issue that this prevents BrainBrew from being able to (even in principle) import changes to the deck name (and the crowdanki_uuid)? (when running anki_to_source)

Not sure I understand what you are saying. That there should be a way to programmatically write to a Header?

(BrainBrew currently doesn't allow writing to the YAML headers file, but in principle it's possible, while if the name and crowdanki_uuid are hard-coded in the recipes then it becomes impossible even in principle.)

Doesn't headers_from_crowd_anki do this? That is what is used in the brain_brew init function to setup a repo. Or am I misunderstanding? 😅

[aplaice]

I think that I'm overthinking this (firstly, changing headers is a very rare use-case to begin with, and secondly changing the name and UUID makes little sense and/or will break other things) so please feel free to ignore the below! (I'm not quite sure why I decided to type all of this up, given that I realised half-way through that it makes no sense...)

---

Monstrously overlong argument

Doesn't headers_from_crowd_anki do this? That is what is used in the brain_brew init function to setup a repo. Or am I misunderstanding? sweat_smile

I meant that currently (at least in AUG's anki_to_source.yaml) changes to the deck header fields (extendNew, extendRev, name) in build/Ultimate Geography [EN]/deck.json don't propagate to src/headers/default.yaml when running recipes/anki_to_source.yaml.

Now actually looking at anki_to_source.yaml that's because save_to_file: src/headers/default.yaml is commented out... (So it's possible both in principle and in practice, but (rightly) commented out because it's rarely used.)

Not sure I understand what you are saying. That there should be a way to programmatically write to a Header?

I meant that now that name and crowdanki_uuid are (at least in some cases) hardcoded in the recipes, they can't (in these cases) be updated when using headers_from_crowdanki, even in principle, in a way that the deck description still could, in principle. (deck_description_html_file is a "pointer", while name and crowdanki_uuid store the raw values.)

deck_description_html_file current behaviour

Currently, when deck_description_html_file is "overriden", then the deck description also can't be programmatically changed. If you change desc in a deck.json, run headers_from_crowd_anki (e.g. like in recipes/anki_to_source.yaml, when you uncomment save_to_file: src/headers/default.yaml), then you end up with desc: changed description in src/headers/default.yaml (I'm using the paths as they are in AUG), but given that you still have

        override:
          deck_description_html_file: src/headers/desc.html

in recipes/source_to_anki.yaml, the changed description (in src/headers/default.yaml) won't ever be used. (So if you change desc, run recipes/anki_to_source.yaml (with save_to_file: uncommented) and run recipes/source_to_anki, you'll end up with your original desc in deck.json. (Obviously, this is without manually modifying in the meantime!))

deck_description_html_file theoretically possible behaviour

(Note: this is not a request for this feature, just for illustration!)

However, one can imagine having an override field in headers_from_crowd_anki (I don't think it's currently possible), for example something like:

  - headers_from_crowd_anki:
    - source: build/Ultimate Geography [EN]
      part_id: default
      save_to_file: src/headers/default.yaml
      override:
          deck_description_html_file: src/headers/desc.html

such that, when you run this headers_from_crowd_anki (as part of a anki_to_source.yaml recipe), the desc field from deck.json is written to src/headers/desc.html. (Consequently, if you change desc in a deck.json and run recipes/anki_to_source.yaml and then recipes/source_to_anki.yaml you still have your changed desc.)

crowdanki_uuid and name

In contrast, given that the values of crowdanki_uuid and name are being stored in the recipe itself, if you were to change the name or crowdanki_uuid in deck.json, there is no way for them to be programmatically updated (in the way that we could, in principle, update the contents of the src/headers/desc.html file).

However, on second thought this doesn't matter.

If the name is changed, then the path to the build directory will have changed (since it's build/${name}/deck.json), so the anki_to_source recipe won't work anyway (without manual editing).

crowdanki_uuid should not ever change, since it's the id identifying the deck.

---

tldr; everything is fine design-wise, ignore the above.

[aplaice]

On a more constructive note, the partial overrides issue is now fixed (there's no crash when only some fields are overridden)!

(The sorting is still as it was: first all the non-overridden fields, then desc and then crowdanki_uuid. I think if we want crowdanki_uuid to be before desc (but after non-overridden fields), then we need to change the order of keys in HeadersOverride.override. If we want all header fields to be sorted. then (I think) we need to use sorted(self.data.items) in Headers.data_without_name.)

[ohare93]

On a more constructive note, the partial overrides issue is now fixed (there's no crash when only some fields are overridden)!

🎉

(The sorting is still as it was: first all the non-overridden fields, then desc and then crowdanki_uuid. I think if we want crowdanki_uuid to be before desc (but after non-overridden fields), then we need to change the order of keys in HeadersOverride.override. If we want all header fields to be sorted. then (I think) we need to use sorted(self.data.items) in Headers.data_without_name.)

Aaaahhhhhh you meant the order in the resulting CrowdAnki file, yes I see. That should be fixable 😄

[aplaice]

AFAICT this works perfectly (as we'd require for #566) with all even minor issues (key sorting) dealt with!

[aplaice]

@ohare93 could you please merge this? (Not urgently!)

(I haven't tested it any more, but from what I remember from anki-geo/ultimate-geography#566 it worked exactly as needed.)

[ohare93]

Huh I never merged this? Whoops! Done 😁 I'll release a new version shortly 👍

[ohare93]

Released 👍 https://github.com/ohare93/brain-brew/releases/tag/v0.3.10

[ohare93]

@omarkohl I see you referenced this PR. It is released now, apologies for the delay! 😅

[aplaice]

Thanks!