brain-brew icon indicating copy to clipboard operation
brain-brew copied to clipboard

Headers Override for Name and CrowdAnkiUUID

Open ohare93 opened this issue 3 years ago • 6 comments

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

For https://github.com/anki-geo/ultimate-geography/pull/566

ohare93 avatar Aug 23 '22 06:08 ohare93

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 avatar Aug 23 '22 10:08 aplaice

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

No worries, any time :grin:

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? 😅

ohare93 avatar Aug 23 '22 11:08 ohare93

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 avatar Aug 23 '22 14:08 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.)

aplaice avatar Aug 23 '22 14:08 aplaice

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

:tada:

(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 :smile:

ohare93 avatar Aug 23 '22 18:08 ohare93

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

aplaice avatar Aug 24 '22 10:08 aplaice

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

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

aplaice avatar Sep 02 '23 09:09 aplaice

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

ohare93 avatar Sep 02 '23 10:09 ohare93

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

ohare93 avatar Sep 02 '23 12:09 ohare93

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

ohare93 avatar Sep 02 '23 12:09 ohare93

Thanks!

aplaice avatar Sep 02 '23 13:09 aplaice