ohare93 / brain-brew

Automated Anki flashcard creation and extraction to/from Csv
The Unlicense
89 stars 5 forks source link

Headers Override for Name and CrowdAnkiUUID #47

Closed ohare93 closed 1 year ago

ohare93 commented 2 years ago

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

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

aplaice commented 2 years ago

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).

ohare93 commented 2 years ago

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

aplaice commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

aplaice commented 2 years ago

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

aplaice commented 1 year ago

@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.)

ohare93 commented 1 year ago

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

ohare93 commented 1 year ago

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

ohare93 commented 1 year ago

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

aplaice commented 1 year ago

Thanks!