Move To Brain Brew #364

ohare93 · 2020-10-04T09:52:19Z

Rather than discuss this further in #143, I am making this PR now so we can go over the still to do tasks before a Pull could happen. This also allows us to see the extent of the changes, in Git's change view.

So, what is still missing? 😁

ohare93 · 2020-10-04T11:15:53Z

I am not an expert of Git Actions, so I will not mess with the previous system that ran the composer index check.

The build recipe can be verified with brain-brew [recipe] --verify and I am open to adding a full --dry-run option. Perhaps just building the deck parts that are saved and checking for diffs would also help.

aplaice · 2020-10-07T22:28:35Z

AFAIR, the ultimate goal is to move to a single data.yaml, with one line per translated field.

However, I think that it might be worth switching to brain-brew's CSV system (as here/in your branch) before then, since it's still (on net) an improvement compared to the single CSV (src/data.csv). Also, the overall tooling won't change significantly when we switch formats "within" brain-brew, so it's not like we'll be forcing our contributors to learn a new system, unnecessarily.

IMO the most important thing missing is having the CSS and templates as separate files.

I have doubts whether per-field CSVs (capital.csv etc.) are better than per-language ones (french.csv etc.) — I can see some advantages of either — but as long as we're planning to eventually switch to the YAML it's probably not worth obsessing over which to choose.

Also, congratulations on (soon) becoming a father!

axelboc · 2020-10-11T11:03:01Z

I also don't mind if we keep the data in CSV files for now. It actually makes the transition smoother I think.

IMO the most important thing missing is having the CSS and templates as separate files.

Yeah, I agree -- merging without this would be a step back, I think. That being said, it's not a huge deal either, so if it postpones the move to BB too much, I wouldn't mind merging without it.

aplaice · 2020-12-13T11:09:20Z

This looks fantastic!

Unfortunately, I won't have time for more than the cursory look I've had (or to test the conversion pathways/recipes) in the nearest future, but I think that it'd be great to get this merged soon!

Three things that might be valuable (but I don't think are essential before merging (not that it's up to me 😃)):

Remove the duplication of data, (particularly src/data/split, src/data/combined_data.csv, src/deck_parts/notes/english.yaml, leaving only the first), to avoid confusing contributors and to avoid stale data.
(Wishlist) have a recipe to "unsplit" the CSVs — in general the huge single CSV is unwieldy, but occasionally it's quite valuable. (i.e. the reverse of legacy_csv_to_split_csvs.yaml).
Update CONTRIBUTING.md.

ohare93 · 2020-12-13T15:06:27Z

This looks fantastic!

Thanks! I just have a quick change to make so the deck description can live as it's own Html file too, and a bugfix about the importing media from CrowdAnki going in the right subfolder instead of the top level media folder. Then I'll release the latest version of Brain Brew, and point UG towards that 👍 (currently still running off my latest local branch).

Unfortunately, I won't have time for more than the cursory look I've had (or to test the conversion pathways/recipes) in the nearest future, but I think that it'd be great to get this merged soon!

No problem mate, but I hope so too!

Three things that might be valuable (but I don't think are essential before merging (not that it's up to me smiley)):

1. Remove the duplication of data, (particularly `src/data/split`, `src/data/combined_data.csv`, `src/deck_parts/notes/english.yaml`, leaving only the first), to avoid confusing contributors and to avoid stale data.

2. (Wishlist) have a recipe to "unsplit" the CSVs — in general the huge single CSV is unwieldy, but occasionally it's quite valuable. (i.e. the reverse of `legacy_csv_to_split_csvs.yaml`).

3. Update `CONTRIBUTING.md`.

The combined_data.csv and the transformer are only there for one last transform of the current state of the data before the proper merge. Though I can leave the recipe there afterwards. Notes can be deleted too, no worries.
Sure, I'll throw that recipe together. Should be no problem.
Can do 👍 if you have anything specific in mind, let me know 👍

aplaice · 2020-12-13T16:07:12Z

Great! :)

Can do +1 if you have anything specific in mind, let me know +1

I was thinking about something equivalent to the "install" and "running" steps from your Brain Brew Starter Readme, to replace the anki-dm-specific information. (Hooray, we'll be mostly able to get rid of the "quote normalisation" issues!) In the long-run we'll probably end up with some brain-brew-specific "best practices" and idiosyncrasies, to add there, but probably not just now. :)

ohare93 · 2020-12-13T17:45:22Z

Alright, all apart from the update to Contributing is now done 👍

I have released Brain Brew 0.2.5, which brings the changes I mentioned above:

Deck Description is it's own Html file now
Existing media will stay in any arbitrary folder inside the "src/media" folder when being overwritten on source_from_anki
Notes can be given an override for the NoteModel (used in the Extended Deck to have the same notes, but a different Note Model, without redoing the work)

I have also added the legacy_csv_from_split_csvs recipe, which took literally 1 min to make 😁 as well as disabled most of the unused save_to_file options for now. The old combined csv remains, but that is because I believe it will be used before the proper merge.

The Extended Deck works, each language is working too. All the html and css files live on their own (though the downside is these lone html and css files cannot be written to in a source_from_anki build just yet, but eventually).

This is a great time for me to update my own Brain Brew documentation, as so much has changed since I last touched it, it is very out of date. I will in tandem update the Contributing here, but would like some sanity checking that everything seems to work first. No rush, it can wait until the new year. I hope Brain Brew will be used in UG by the end of January! 😁 👏

Lemme know if you guys spot anything, whenever you have time. Thanks again for your hard work 👍

aplaice · 2020-12-13T17:53:10Z

Wow, that was amazingly fast! I'll try to have a detailed look ASAP and definitely before the New Year (I hope...)!

ohare93 · 2020-12-13T18:18:54Z

Wow, that was amazingly fast!

Only a few months! 😆 I finally had a good amount of free time today, for once in the last few months 😉

ohare93 · 2020-12-13T18:19:38Z

Oh, I should of course do a pull of the latest UG again. And I am sure an update to the Workflow files would be appreciated too.

ohare93 · 2020-12-19T08:38:22Z

Rebased onto latest, and updated the workflow to do a full build and check for errors 👍

Just have to make changes to the Contributing. At a glance I can see I should:

Update Setup
Update Build and Import
Remove Indexing
Remove Quote normalisation
Update Release process
Make Brain Brew specific notes
- The files being separated
- Guids are created automatically on build
- Link to documentation on recipe files
- Running source_to_anki or source_from_anki
- Steps to add a new language

See anything else I should change? 😁 I'll get on this in a week or so, moving house today/tomorrow 🙈

@axelboc

(As noticed by @axelboc.)

It only converts from the English crowdanki deck.

legacy_csv_from_split_csvs recipe

(Note that Nur-sultán and Macau were fine in the split CSVs.)

ohare93 · 2021-01-02T19:06:53Z

The issue would be that a GUID is generated, by recipes/source_to_anki.yaml and entered in the CrowdAnki json, just not fed back into the CSV. Hence, CrowdAnki would happily import the deck and the new note, into Anki, but with a GUID whose value was not registered in main.csv, and if a GUID were later generated in the CSV, it wouldn't match the one in Anki. (Obviously, deleting the new "bad-GUID" notes in Anki, or manually copying the GUID(s) from the JSON to the CSV, would normally not be an issue, but it might be confusing and/or inconvenient.)

Oh shit, you're right, the guid gets generated automatically regardless 🤔 hmmmmmmm. I suppose I should make the notes_from_csv task stop generating the guids automatically, and have it return an error. Alright, this is an easy fix 👍 Will get to it soon.

Fix Kyiv (from recent updates) in capital.csv

ohare93 · 2021-01-10T15:03:19Z

All changes made 👍

The files should be in the locations discussed 👍

The Guid generation works when running either to or from source 👍 (side effect: now all the child csvs are ordered alphabetically, and will continue to update themselves to this whenever you run one of the recipes. Can be changed 👍 )

The Legacy Csv is now in a Gist https://gist.github.com/ohare93/0496ec1624d72abeb2085cb640d5528a 👍 though I had some issue getting the Legacy recipes to work without the actual file. Long story short, the csv builder needs to know what the headers are before it can populate the file. I'll make a generic task to get around this later on, but not now, since this is not that high priority. If you want to use the legacy file you just need to make src/data/combined_data.csv and add the current headers, then everything should work fine 👍

Remember to update your version of Brain Brew in your own repo of this, if you wanna try it out 👍 I hope we can move forward and get this rolling! 😁 new translations and extension repos await! 😁

axelboc

Fine by me! Ready to merge when @aplaice gives his green light 👍

src/note_models/style.css

CONTRIBUTING.md

src/data/country.csv

aplaice · 2021-01-10T18:34:39Z

This looks great! I can't wait for the new translations and for the extensions! :)

One more thing that we all somehow managed to miss is that the wrong templates were picked for the non-extended deck! We want the Country - Capital, Capital - Country, Flag - Country, and Map - Country notes rather than the Country - Capital, Capital - Country, Country - Flag, and Country - Map ones! (Unless we're celebrating switching to our brilliant new system, by increasing the difficulty of the deck, a notch! :) )

(I only noticed this when comparing our old build output with the new one (~ diff <(jq --sort-keys < old_build/Ultimate\ Geography/Ultimate\ Geography.json) <(jq --sort-keys < build/Ultimate\ Geography/Ultimate\ Geography.json) (the output is a bit messy since only dicts are sorted, not lists)...)

(Edit: Also, the order of note templates has been changed in the extended deck, which would probably jumble up people's review history, if imported on top of an existing AUG deck.)

(Both fixed here.)

Edit 2: I've also only now noticed that the deck names are now all "Ultimate Geography" (rather than "Ultimate Geography[xx]" or "Ultimate Geography [Extended]"). This is definitely not a blocker and I'm not actually certain which "style" I prefer.... (Using just "Ultimate Geography" for all the decks has the slight disadvantage that it might lead to some slight inconsistencies when re-exporting with CrowdAnki, since the newly generated folders/JSONs will always be called Ultimate Geography/deck.json rather than Ultimate Geography[xx]/... etc., but that's not really important.)

(To avoid making the deck harder and to avoid confusing review history.)

Use same templates and same template order as previously

ohare93 · 2021-01-10T19:17:48Z

(Both fixed here.)

Lovely, thank you. That was my bad 😅

Edit 2: I've also only now noticed that the deck names are now all "Ultimate Geography" (rather than "Ultimate Geography[xx]" or "Ultimate Geography [Extended]"). This is definitely not a blocker and I'm not actually certain which "style" I prefer.... (Using just "Ultimate Geography" for all the decks has the slight disadvantage that it might lead to some slight inconsistencies when re-exporting with CrowdAnki, since the newly generated folders/JSONs will always be called Ultimate Geography/deck.json rather than Ultimate Geography[xx]/... etc., but that's not really important.)

Ahh yes, as they all use the same Header. Hmm 🤔 I agree it would be good to have an override for the name here, but fyi the source_from_anki recipe currently only looks at the deck Ultimate Geography so users of the translations must change the recipe to get the importing back into source. An improvement I'd like to do on this in the future is to have the deck name be Regex, and the builder offers all options that match in the chosen folder for selection mid run. But that is neither here nor there, this original deck name issue can be fixed easily in the future 👍

aplaice · 2021-01-10T19:21:50Z

Ahh yes, as they all use the same Header. Hmm thinking I agree it would be good to have an override for the name here, but fyi the source_from_anki recipe currently only looks at the deck Ultimate Geography so users of the translations must change the recipe to get the importing back into source. An improvement I'd like to do on this in the future is to have the deck name be Regex, and the builder offers all options that match in the chosen folder for selection mid run. But that is neither here nor there, this original deck name issue can be fixed easily in the future +1

Yeah, given that the Anki → CSV contribution pathway is still relatively immature (we didn't have this available at all until now! :)) it's totally not an issue; I had simply noticed the (harmless) deck name change and commented!

ohare93 · 2021-01-10T19:23:36Z

@aplaice no problem mate, thank you for being so thorough 🤝

Alright, any other stragglers? Can we push the button?! 😱

aplaice · 2021-01-10T19:27:48Z

I think it's just the CSS (ideally —it's not really a blocker) and we can push the button (if GitHub allows us*).

* it's not happy that it has run only one test and it has grayed out the merge button...

(I'm trying to figure out how to get around this without merging via CLI git, which would feel very unsatisfying...)

ohare93 · 2021-01-10T19:32:37Z

CSS reverted 👍

aplaice · 2021-01-10T19:33:07Z

Great, thanks, hooray! I'm voting for merging/green light given :p!

aplaice · 2021-01-10T19:52:33Z

I'm still stumped by GitHub requiring two passing tests — after all, we've deliberately reduced the number of tests...

Edit: AFAICT @axelboc, as administrator, will still be able to merge through the web interface. (I'm not able to, but I wouldn't have dared anyway. :D)

axelboc · 2021-01-10T21:38:32Z

Sorry for the delay 😄

Amazing effort, @ohare93. 💯 Thanks a lot for getting this done!!! The future is bright. 😄

ohare93 · 2021-01-11T05:58:27Z

💯 🥳 ❗

2 checks passed

What magic did you do?! 😆

axelboc · 2021-01-11T07:59:05Z

Haha no clue! 😄 You never know what's going to happen when you add/modify/remove a workflow in a PR...

axelboc mentioned this pull request Oct 11, 2020

Investigate better internationalisation format/system/workflow #143

Closed

axelboc mentioned this pull request Dec 18, 2020

Discussions Tab #367

Closed

ohare93 force-pushed the move-to-brain-brew branch from 2bdcf04 to e52b243 Compare December 19, 2020 08:17

ohare93 mentioned this pull request Dec 21, 2020

Added Russian language #363

Merged

ohare93 and others added 15 commits December 23, 2020 09:14

Merged Latest

3b035b5

Setup with Old to New format transformer

255bc82

Full Standard Deck conversion functional

fec4512

Update Pipfile to use latest Brain Brew V0.2.2

eb6782e

Fix German capital hints

ceccef8

(As noticed by @axelboc.)

Finish off (now apparently working) source_from_anki.yaml

0861315

It only converts from the English crowdanki deck.

Brain Brew V0.2.4

782ac9e

Source to Anki Update to Latest BB

444c917

Source from Anki Update to Latest BB

1b85524

Separation of Note Model items: CSS, Fields, and Templates

a781833

Tidy Up

7f93309

Deck Description as HTML file;

409e871

legacy_csv_from_split_csvs recipe

Using Release of Brain Brew 0.2.5

0fb7af9

Rebase onto latest

4c881e0

Workflow updates

9d30888

aplaice added 2 commits January 2, 2021 19:31

Fix Kyiv (from recent updates) in capital.csv

08e9005

Fix src/data/combined_data.csv

25e70e6

(Note that Nur-sultán and Macau were fine in the split CSVs.)

ohare93 added 4 commits January 2, 2021 20:07

Merge pull request #9 from aplaice/mbb_kyiv

5de1082

Fix Kyiv (from recent updates) in capital.csv

Move files to correct locations

3afca3a

Update to Brain Brew 0.2.6

3c4516e

Update requirements.txt

548f89f

axelboc approved these changes Jan 10, 2021

View reviewed changes

src/note_models/style.css Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

src/data/country.csv Show resolved Hide resolved

brain_brew_build -> build

54f6462

aplaice and others added 2 commits January 10, 2021 19:42

Use same templates and same template order as previously

53adcc2

(To avoid making the deck harder and to avoid confusing review history.)

Merge pull request #10 from aplaice/mbb_note_templates

4079ad7

Use same templates and same template order as previously

Revert CSS

ba2c337

aplaice approved these changes Jan 10, 2021

View reviewed changes

aplaice closed this Jan 10, 2021

aplaice reopened this Jan 10, 2021

axelboc merged commit 7df0264 into anki-geo:master Jan 10, 2021

axelboc added the chore Documentation, licenses, repository structure, dependency upgrades, etc. label Jan 20, 2021

axelboc added this to the v4.1 milestone Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move To Brain Brew #364

Move To Brain Brew #364

ohare93 commented Oct 4, 2020

ohare93 commented Oct 4, 2020

aplaice commented Oct 7, 2020

axelboc commented Oct 11, 2020

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020 •

edited

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020

ohare93 commented Dec 13, 2020

ohare93 commented Dec 19, 2020 •

edited

ohare93 commented Jan 2, 2021

ohare93 commented Jan 10, 2021 •

edited

axelboc left a comment

aplaice commented Jan 10, 2021 •

edited

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021 •

edited

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021 •

edited

aplaice commented Jan 10, 2021 •

edited

axelboc commented Jan 10, 2021

ohare93 commented Jan 11, 2021

axelboc commented Jan 11, 2021

Move To Brain Brew #364

Move To Brain Brew #364

Conversation

ohare93 commented Oct 4, 2020

ohare93 commented Oct 4, 2020

aplaice commented Oct 7, 2020

axelboc commented Oct 11, 2020

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020 • edited

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020

aplaice commented Dec 13, 2020

ohare93 commented Dec 13, 2020

ohare93 commented Dec 13, 2020

ohare93 commented Dec 19, 2020 • edited

ohare93 commented Jan 2, 2021

ohare93 commented Jan 10, 2021 • edited

axelboc left a comment

Choose a reason for hiding this comment

aplaice commented Jan 10, 2021 • edited

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021 • edited

ohare93 commented Jan 10, 2021

aplaice commented Jan 10, 2021 • edited

aplaice commented Jan 10, 2021 • edited

axelboc commented Jan 10, 2021

ohare93 commented Jan 11, 2021

axelboc commented Jan 11, 2021

ohare93 commented Dec 13, 2020 •

edited

ohare93 commented Dec 19, 2020 •

edited

ohare93 commented Jan 10, 2021 •

edited

aplaice commented Jan 10, 2021 •

edited

aplaice commented Jan 10, 2021 •

edited

aplaice commented Jan 10, 2021 •

edited

aplaice commented Jan 10, 2021 •

edited