Giter Site home page Giter Site logo

Feedback from UG set-up about brain-brew HOT 16 CLOSED

ohare93 avatar ohare93 commented on May 30, 2024 1
Feedback from UG set-up

from brain-brew.

Comments (16)

ohare93 avatar ohare93 commented on May 30, 2024 4

Note Model is now separated 🥳 CSS and Templates live in their own files, and Fields are done through Yaml. Again, see the latest in move_to_brain_brew for the details.

Just have some fixes to do to allow easy generation of the extended deck (is currently working, but requires duplication of Note generation step 😠) then I'll release the latest and give y'all a proper update in the move_to_brain_brew branch 👍

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 2

Sorry for the late reply, I have been rather busy. Will become a dad in the next month or so, so plenty to do! 😁 🎉


I like scripts because it's very generic and quite a common name, but I also like .brain-brew, which is the complete opposite. Tooling folders like the latter seem to be becoming a convention of sorts, and they have the merit to be clear.

How about a recipe? 😉 this Brain-Brew after all! 😁 Though I would say the files should still be .yaml so that code editors can give highlighting and autocomplete for yaml specific keys 👍


The way deck parts are identified in memory through name is not very explicit in my opinion. That's why, in my example, I suggest using a key called part_id -- it's clear that it's an identifier that is meant to be used later on, and it also clarifies what the output of the transformer is (i.e. a deck part in memory).

Good suggestion, I shall adopt this immediately 👍


I'm back to what I was saying about a task/transformer doing both read and write operations. In my opinion, a transformer should only do one and be named accordingly: notes_from_csv already parses notes from CSV files; it shouldn't also write to a file (c.f. single-responsibility principle). Why not have dedicated transformers take care of writing each type of deck part to YAML?

- Save notes to YAML
  transformer: notes_to_yaml
  output_file: build/english.yaml
  notes: english

I get what you're saying, but this comes with a few downsides. First of all, the first line here would not be valid, as the rest is a dictionary, so this would either have to be made into an explicit comment key, or just a yaml comment itself. Which we can already do, I guess 😅 Secondly, having transformer be a key inline with the rest of the keys complicated type prediction when running prechecks to see if all the expected keys are present. It is still possible to do so with this structure, but it is more difficult, I believe. Also, it creates wasted lines of repetition, in my opinion, where a nested format would be simpler. For instance, the following:

- generate deck parts:
  - from deck parts:
      note_models:
        - name: Ultimate Geography
          file: src/deck_parts/note_models/Ultimate Geography.yaml
        - name: Ultimate Geography Combined Fields
          file: src/deck_parts/note_models/Ultimate Geography Combined Fields.yaml

would turn into

- generate deck parts:
  - from deck parts:
    - transformer: note_model
      name: Ultimate Geography
      file: src/deck_parts/note_models/Ultimate Geography.yaml
    - transformer: note_model
      name: Ultimate Geography Combined Fields
      file: src/deck_parts/note_models/Ultimate Geography Combined Fields.yaml

and the extra lines would only compound as more parts / transformers are used 😓 This also would not work for tasks which take lists. I will keep the nesting as it is for now, but ofc anyone can point out something I missed 😁


The verb "generate" for a task that puts something in memory feels wrong

Hmm 🤔 I get that. Do you have a better suggestion? create_deck_parts, build_deck_parts?


Edit: actually it seems that save_to_file is also used for the "final" writing-to-file, for instance in source_from_anki.yaml, which feels slightly weird.

I understand your confusion about this key. The exact same result could be achieved by having a separate write deck parts key which takes a list of part_ids and their write location, and then writes those to disk. Though I just felt that this was rather clunky, as all deck parts are immutable so their content will never change due to being used by other tasks. So writing them at the end of the process is unnecessary, though I agree would perhaps add clarity when reading the task list itself.

I will not change this behaviour at the moment, but anyone else can feel free to implement a write deck parts top level task 👍 which should be quite easy.


I agree with @axelboc and also prefer unambiguous task names (for instance, it makes it easier to grep the source code for references :)). OTOH flexible names are indeed cool and neat, so perhaps there could be a "canonical" name, while also allowing matching by regex?

Hmm, you both have swayed me. This is indeed an incredibly unimportant feature, that I have already spent too much time on, compared to the other large tasks that still exist. Standard expected keys will suffice. However I will leave the implementation using Regex for now, so that improvements can be made here in the future.

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 2

Changes made! See my PR for all the nitty gritty details: #13 I just went with my best judgement from out conversation, but nothing is permanent 👍

I have updated the Move-To-Brain-Brew branch of UG to reflect these changes 👍 Which you can find here ohare93/anki-ultimate-geography@ab83d80

Remember to run pipenv install again, to update to the latest version of Brain Brew!

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 2

You mention that users would group all deck part building tasks at the top of the recipe, but what's the harm in interleaving them with generate tasks? Someone may want to have a recipe that builds a deck part, then does something with it, then builds another deck part and does something else with it. I don't know, I feel like it's just an unnecessary constraint.

That is my expectation, yes. But as I said earlier, there is no limitation here. Someone can use multiple build_deck_parts tasks, or multiple from_deck_parts.

Side note, I'd recommend at least renaming from_deck_parts to from_yaml_deck_parts to convey the source format. Someone may want to store raw deck parts in another format one day. man_shrugging

Not a bad idea 👍

Regarding from_deck_parts > note_models and the like, it's not so much about the flattening per se, but about the cognitive simplicity of one transformer having one input and one output (i.e. one deck part, not multiple deck parts).

But the same is not true about other transformers, like deck_parts from_crowdanki:

- build_deck_parts:
  - from_crowd_anki:
      folder: brain_brew_build/Ultimate Geography/
      notes:
        part_id: standard notes
        sort_order: [Country]
        save_to_file: src/deck_parts/notes/english.yaml
      note_models:
        - part_id: Ultimate Geography
          save_to_file: src/deck_parts/note_models/Ultimate Geography.yaml
      headers:
        part_id: default
        save_to_file: src/deck_parts/headers/default.yaml
      media:
        from_notes: true
        from_note_models: true

One can create Notes, multiple NoteModels, Headers, and Media. Would you also suggest breaking this up into multiple? 🙁

Hmm, I thought I would be more against this, but looking back on my reasoning I am seeing some misalignment.

My main reason was to limit repetition which could lead to mistakes, but with Yaml Anchors that can be mitigated. Further weight behind your argument is that my actual code is written in this atomic structure that you are advocating for. Before my rewrite there was a lot more interdependencies between the different components, and the final one remaining (Notes need to know the names of the Note Models) can be fixed with no worries.

I think that smaller, atomic transformers called note_model_from_deck_parts, or note_from_csvs, would be self-descriptive.

God damn it, you've convinced me 😅 😆 I will work on this immediately. Better to get this stuff nailed down while nobody is using the program! 😄

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 2

I'm back to it 💪 Atomisation complete 👍

  • Build parts are now much more atomic
  • Media files are read in groups, and can change for each export.
    • It is also possible to combine or subtract media groups before exporting them
  • Yamale is now used to validate Yaml

See pull #14 for the code changes.

I have also updated the Move to Brain Brew branch of UG ohare93/anki-ultimate-geography@903b21e Here'ss an example of a difference:

Old

- build_deck_parts:
  - from_crowd_anki:
      folder: brain_brew_build/Ultimate Geography/
      notes:
        part_id: standard notes
        sort_order: [Country]
        save_to_file: src/deck_parts/notes/english.yaml
      note_models:
        - part_id: Ultimate Geography
          save_to_file: src/deck_parts/note_models/Ultimate Geography.yaml
      headers:
        part_id: default
        save_to_file: src/deck_parts/headers/default.yaml
      media:
        from_notes: true
        from_note_models: true

New

- build_parts:
  - notes_from_crowd_anki:
      source: brain_brew_build/Ultimate Geography/
      part_id: standard notes
      sort_order: [Country]
      save_to_file: src/deck_parts/notes/english.yaml
  - note_models_from_crowd_anki:
    - source: brain_brew_build/Ultimate Geography/
      part_id: Ultimate Geography
      save_to_file: src/deck_parts/note_models/Ultimate Geography.yaml
  - headers_from_crowd_anki:
    - source: brain_brew_build/Ultimate Geography/
      part_id: default
      save_to_file: src/deck_parts/headers/default.yaml
  - media_group_from_crowd_anki:
    - source: brain_brew_build/Ultimate Geography/
      part_id: all_anki_media
  - save_media_group_to_folder:
      parts: [all_anki_media]
      folder: src/deck_parts/media
      clear_folder: false

I will release this new version tomorrow so others can try it, so only local installs will work until then, fyi 👍 Anyways, glad to be back! 😁 I will not work on the NoteModel parts being separate files 🎉

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 1

Thanks for the feedback, let me take these in the order that makes most sense to me:

Structural changes

  1. brain_brew_config.yaml - very minor, but what would you think of calling this file brain-brew.config.yaml?

The global config file will soon be fully removed, I hope. It is no longer useful, in my eyes, and I wish for everything that it does to be configurable in the builder files yaml files that define the tasks of how stuff is made 😏 So the media super-folder will soon be definable per task, so different decks can even use replacement media 👍

  1. Perhaps it's your plan, but I feel like some of the deck parts could be split up even more so they could live in separate files like in AnkiDM -- I liked how templates and the deck's description lived in HTML files, the styles in a CSS file, etc.

That's the plan, indeed! I would like to have css and html be their own files too, but I'd like that to be optional, not necessary. The current issue is that I don't know how I should structure the files after this, while trying to maintain the ability to merge to and from crowdanki 🤔 the templates are either used or not, no worries there, but the for the css I think having a bunch of separate css files which can be combined together would be best. This way an extended deck can add just one css part to get some new functionality in their new card, or can overwrite just a specific part in order to change some functionality. All the while they keep the most recent changes in the main repo, with the minimum changes propagating out into them (and less work for us 😅 )

Just spit balling ideas, lemme know if anyone has a better suggestion 😁👍

Names of stuff

  1. builder_files - I know the naming of this folder is not tied to Brain Brew, but I feel like scripts would be a better name. The way I see it, the YAML files in this folder are "scripts" that define "tasks".

In the sense of what I call things in the code I've pretty much been winging it on the names, just picking whatever makes sense in my head. Absolutely nothing in the system needs to be called that, and can be changed to whatever makes more sense 👍 A "Builder" made sense to me as it 'builds' the stuff, but yes I can also see a "script" also works 👍 I'll have a think about it, and would like to hear what others think.

However the other way to look at this is: what do we call the folder in the UG repo? Call it literally whatever you want 😁 as long as you run it in the command line or point to it in the script, s'all good. Same with every folder in any repo that uses UG. The global_config can even be renamed, as long as you run brain brew with -g name_of_config 👍

  1. generate syntax - in the YAML builder files (scripts?), I find the generate syntax a bit confusing, as it encompasses both read and write operations (sometimes both in the same task, with save_to_file).

Same goes with these! The term "Generate" just makes sense to me, as it makes those things, however this one is entirely superfluous. generate csv as a top level key in the Yaml matches the CsvGenerate task in the code only because the key matches the regex .*csv.* 😄 Same goes for generate crowdanki or make me a crowd anki pls as it matches .*crowd[\s_-]*?anki.* which you can find here in the code. My intention here is to have these keys be flexible in that users can put whatever makes sense but still have it match an actual transform task. So something like english csv may make sense in this case, and would still work! 😁 I just did not want to limit users to a specific key set, which may change in the future, and that others may not like whatever naming convention is/was used, like Generate_Csv_Collections 😅 this way, you are free to do it yourself, as long as it matches only 1 of the tasks!

Deck Parts are the only special one right now, as the name is a top level task and it also matches each of the other types in it's children (their X_from_source type, such as notes_from_crowdanki). So these are all equivalent:

generate deck parts:
   - from csv:
        ...
   - csvs:
        ...
   - country_csv:
        ...

and the top level again does not need the term generate, though it is what the task does, it makes deck parts. The second reason deck parts are special is that they do not save as a file by default, and are merely held in memory so that other files can be made with them. One can save them to memory if they wish, but it is optional.

I feel like having separate, independent read (i.e. parsing a file/folder) and write (i.e. writing to a file/folder) tasks would make things a lot clearer.

It's a little more complicated than that, as a task does not simply read or write, usually both are done as it is more of a transform of one data into another, each time. Csv <-> Deck Parts <-> Crowd Anki. The only step that is just a read is reading a Deck Part from the local Yaml representation, as in

generate deck parts:
   from deck part:
      ...

So I chose for everything to be a generation task, you make a deck part, you make a csv, you make a crowd anki (and more to come) and that is the entirety of the top level key definitions. Also the 'read' tasks, which make the non deck part file tasks have no concept of the files in the system used previously. Only Deck Parts is a working list that is kept in memory, and can be referenced in other tasks. That's how I designed the system to be reliant on Deck Parts, as I want each source type to be entirely independent of the others.

  1. Deck parts is what you call the various pieces of a deck's metadata, right? Would you be able to write down a list somewhere of all the deck parts that you have defined/identified, just so we're clear?

The Deck Parts to me are: Notes, Note Models, Headers, and Media. I go into detail on these here #4 (comment)

I would like to remove Headers entirely as a requirement, but that will take some more work in CrowdAnki in order to get to there. I believe you agree with me on that goal 😅 For the time being they are a necessary evil.

One can ofc break down Note Models into further smaller components, such as Templates and Fields, but I think they are related enough to be a part of the same general thing, even if they end up being stored in separate files. I hope to add in a filter for these two fields into Brain Brew soon, in the form of a whitelist or blacklist, so that one can choose to only use the Fields and Templates that you want (when doing a transform in either direction, that is). This will hopefully also be an option in CrowdAnki itself, which may remove the requirement of generating an Extended deck in the first place 🤔 😁 the user simply gets the option in CrowdAnki to include them or not (and I am sure we can set the default to be without the extra two templates).

from brain-brew.

aplaice avatar aplaice commented on May 30, 2024 1

That definitely looks cleaner!

I hope you're a happy dad now, though I'm also glad that you're still finding time to work on this! :)

from brain-brew.

axelboc avatar axelboc commented on May 30, 2024 1

Great work!! It's looking really good. 💯

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024 1

I guess this issue is resolved! 😁

from brain-brew.

axelboc avatar axelboc commented on May 30, 2024

but the for the css I think having a bunch of separate css files which can be combined together would be best

Totally! The blurring of the flags could live in its own file, for instance.

However the other way to look at this is: what do we call the folder in the UG repo?

I like scripts because it's very generic and quite a common name, but I also like .brain-brew, which is the complete opposite. Tooling folders like the latter seem to be becoming a convention of sorts, and they have the merit to be clear.


generate csv as a top level key in the Yaml matches the CsvGenerate task in the code only because the key matches the regex .csv.

Riiiight, I get it now! Pretty cool, actually. 🙂

A part of me can't help but wonder, though, whether it wouldn't be safer and clearer to know exactly which transformer is run. I'm thinking that if transformers were to become more specific or cross-purpose, regexes could become confusing or run into conflicts. A clearer identification would also:

  • make it a lot easier to find the transformer's source code and documentation;
  • avoid relying on people writing good, meaningful task names.

In my opinion, you could get the best of both worlds with something like:

- Task name that can be anything
  transformer: my_transformer
  # ... (rest of config)

I also wonder whether the deck-part-parsing "sub-tasks" (e.g. - from csv:) couldn't just be top-level tasks in their own right, with their own, purpose-built transformers -- something like:

- Parse note model from YAML
  transformer: note_model_from_yaml
  part_id: ug
  # ...

- Parse English notes from CSVs
  transformer: notes_from_csv
  part_id: english
  # ...

- Parse German notes from CSVs
  transformer: notes_from_csv
  part_id: german
  # ...

# etc., then the "output" tasks:

- Generate CrowdAnki JSON
  transformer: all_deck_parts_to_crowdanki
  output_folder: build/Ultimate Geography
  note_model: ug
  notes: english
  # ...

We could then come up with a naming convention for the transformers, like:

  • <deck_part_type>_from_<format> for deck parts parsers;
  • <deck_part|"all_deck_parts">_to_<format> for generators that take deck parts and output files.

The example above packs a lot of suggestions. I've probably over-simplified things since I don't have a full picture of all the use cases you're trying to cover, but do you see where I'm getting at? I should probably justify a bit more where the above syntax came from... 😅

Basically, in my opinion, what I'm suggesting would clear up a number of confusions, and notably the following:

  • - from csv:
      notes:
        # ...

    The notes object adds a level of nesting, and is most often used on its own. By having dedicated transformers to parse each type of deck part from each file format that can store them, the whole thing can be declared in a single line, with zero extra level of nesting: transformer: notes_from_csv.

  • - generate deck parts:
      - from deck parts:
        #...
    • Some deck parts (note models and headers) can be stored in YAML files, but they end up as deck parts in Brain Brew's runtime memory as well. I had a lot of trouble getting my head around this because of these two lines.
    • The verb "generate" for a task that puts something in memory feels wrong (see item below about save_to_file).
    • from deck parts doesn't convey anything about the file format.
  • notes:
      name: english

    The way deck parts are identified in memory through name is not very explicit in my opinion. That's why, in my example, I suggest using a key called part_id -- it's clear that it's an identifier that is meant to be used later on, and it also clarifies what the output of the transformer is (i.e. a deck part in memory).

  • save_to_file: src/deck_parts/notes/english.yaml

    I'm back to what I was saying about a task/transformer doing both read and write operations. In my opinion, a transformer should only do one and be named accordingly: notes_from_csv already parses notes from CSV files; it shouldn't also write to a file (c.f. single-responsibility principle). Why not have dedicated transformers take care of writing each type of deck part to YAML?

    - Save notes to YAML
      transformer: notes_to_yaml
      output_file: build/english.yaml
      notes: english

Wow, that was a lot, sorry. 😄 I hope there's something in there that makes sense! 😅

from brain-brew.

aplaice avatar aplaice commented on May 30, 2024

I haven't fully digested @axelboc's suggestions, but some partially thought-out, randomly-ordered (sorry!) responses.

Before diving in, none of the issues are IMO "breaking" and bikeshedding syntax should probably be relatively low in priority, at the moment. (Even though in the long run, getting syntax right is important.)

generate csv as a top level key in the Yaml matches the CsvGenerate task in the code only because the key matches the regex .csv.

I agree with @axelboc and also prefer unambiguous task names (for instance, it makes it easier to grep the source code for references :)). OTOH flexible names are indeed cool and neat, so perhaps there could be a "canonical" name, while also allowing matching by regex?


It's a little more complicated than that, as a task does not simply read or write, usually both are done as it is more of a transform of one data into another, each time.

Is this for example when you're writing a set of fields/columns to a CSV, which also contains other fields/columns which aren't being modified? (As would presumably be the case when converting a single language CrowdAnki deck to CSV.)


save_to_file

I admit that I like this key, even though it mixes reading and writing (and is "impure"/results in "side effects"), as it's a neat way of caching the intermediate data (the "deck part").

I don't think that it should really be used "in production", since IMO there should only be a single source of truth, but I imagine that it might be extremely valuable for quick debugging.

Edit: actually it seems that save_to_file is also used for the "final" writing-to-file, for instance in source_from_anki.yaml, which feels slightly weird.


The way deck parts are identified in memory through name is not very explicit in my opinion. That's why, in my example, I suggest using a key called part_id

I found name as the name of the "id" to be OK. part_id or even just id would have also been OK.

Perhaps, for clarity, there could be an (unenforced) convention on identifier names (just like there is for variable names in some programming languages)? For instance ALL_CAPS or _underscore_prefixed or suffixed_? I'm not sure if it's worth it, though.

(Off-the-cuff-thought: As a far more disruptive suggestion, we could have the name as the top-level, e.g.

- Ultimate Geography:
  - from deck parts:
      note_models:
        - file: src/deck_parts/note_models/Ultimate Geography.yaml

according to the idea that what we're doing is defining "Ultimate Geography" for later use. ("Ultimate Geography = ..."). Obviously, there'd need to be a way of disambiguating between identifiers and writing-to-end-files transformations.

Aaah, but this wouldn't work in the case of CrowdAnki → CSV transformations, since we'd need the equivalent of (model_name, notes_name, headers_name) = ..., which wouldn't work with YAML, at least not without further modifications.)

from brain-brew.

axelboc avatar axelboc commented on May 30, 2024

Sorry for the late reply, I have been rather busy. Will become a dad in the next month or so, so plenty to do! 😁 🎉

Wow, congrats!! 👶 🍼

How about a recipe? 😉 this Brain-Brew after all! 😁

Love it! 😁

First of all, the first line here would not be valid, as the rest is a dictionary [...]

Right, silly me, the syntax I had in mind was more like that of GitLab CI:

Save notes to YAML:
  transformer: notes_to_yaml
  output_file: build/english.yaml
  notes: english

Does dropping the top-level array syntax pose a challenge? I'm not all that familiar with the inner-workings of YAML (as you may have guessed 😄), so perhaps the array provides better ordering guarantees?

Secondly, having transformer be a key inline with the rest of the keys complicated type prediction when running prechecks to see if all the expected keys are present. It is still possible to do so with this structure, but it is more difficult, I believe.

Makes sense, I get you. Anyway, as you said, it doesn't matter if the transformer name is the top-level key since it's easy to add a comment above it! https://tenor.com/view/smart-so-sosmart-gif-15534259

Also, it creates wasted lines of repetition, in my opinion, where a nested format would be simpler.

I slightly disagree on this one: I think that repetition can make things clearer if it's the right kind of repetition. Repeating the transformer, in my opinion, is a good thing because it makes identifying all the transformations that take place easier. If this leads to config repetitions across multiple tasks, then this could be addressed specifically (i.e. by providing a syntax to share config between tasks at the top-level, for instance).

That being said, allowing multiple tasks to be defined as an array under the same transformer is conceptually sound, so this is more of a philosophical discussion. 😄

Changes made!

Awesome! part_id really makes things clearer, and build_deck_parts has a lot less of a file-writing vibe. 💯


Looking back at the updated build_deck_parts syntax, I still think this should be flattened or rethought somehow 🤔:

- build_deck_parts:
  - from_deck_parts:
    note_models: ...
    headers: ...
  - from_csvs:
    notes: ...

(I think) I fully understand the syntax now, so I'm happy to work with it for the time being. However, I do feel like the from_ keys interfere with readability. To me, a couple of more readable alternatives could be:

  • - build_deck_parts > - note_model > - from_deck_parts,
  • - build_deck_parts > - note_model_from_deck_parts,
  • - build_note_model > - from_deck_parts,
  • - build_note_model_from_deck_parts.

If I'm not mistaken, currently, a build_deck_parts "sub-transformer" (like from_deck_parts) can potentially handle multiple types of deck parts. The alternatives I suggest would basically require splitting the sub-transformers so that one sub-transformer can build only one type of deck part from one source format. Is this right, and if so, is this something you would consider doing in the long term?

Note that you could still nest multiple tasks under the same transformer if you wanted:

- build_deck_parts:
  - note_model_from_deck_parts:
    - part_id: Ultimate Geography
      file: src/deck_parts/note_models/Ultimate Geography.yaml
    - part_id: Ultimate Geography Combined Fields
      file: src/deck_parts/note_models/Ultimate Geography Combined Fields.yaml

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024

That being said, allowing multiple tasks to be defined as an array under the same transformer is conceptually sound, so this is more of a philosophical discussion. smile

😆 I get your point about certain types of repetition though, that makes sense. Let's file this away as a bridge to cross after a bunch of bigger breaking changes fundamentally change the structure of the whole program again 😅


Looking back at the updated build_deck_parts syntax, I still think this should be flattened or rethought somehow thinking:

🤷‍♂️ not much difference in the end result here. Why do you prefer this flattened structure in this instance though? 🤔

- build_deck_parts:
  - from_deck_parts:
      note_models:
        - part_id: Ultimate Geography
          file: src/deck_parts/note_models/Ultimate Geography.yaml
      headers:
        - part_id: default header
          file: src/deck_parts/headers/default.yaml

I made it the way it currently is as I had (have?) the expectation that a user will only ever perform 1 instance of reading all the deck parts, at the start of the Recipe. "Why split it up into multiple, if they'll just group them all together anyways?" was my thought 🤔 One can always make multiple from_deck_part tasks, each with a subset of the deck parts, though I cannot foresee a use case for that 🤔

Anyways not explicitly against this change, but it will be low priority without further convincing 😅

from brain-brew.

axelboc avatar axelboc commented on May 30, 2024

Fair enough 😅

Regarding from_deck_parts > note_models and the like, it's not so much about the flattening per se, but about the cognitive simplicity of one transformer having one input and one output (i.e. one deck part, not multiple deck parts). The flattening is something that can be gained from this simplicity.

If you take the names of the deck part transformers on their own (from_deck_parts, from_csvs, etc.), they give me no information as to which deck part they are able to build. They are too generic and vague. Can from_csvs build note_models? To me this is a clue that something's not right. I think that smaller, atomic transformers called note_model_from_deck_parts, or note_from_csvs, would be self-descriptive.

Side note, I'd recommend at least renaming from_deck_parts to from_yaml_deck_parts to convey the source format. Someone may want to store raw deck parts in another format one day. 🤷‍♂️

Regarding build_deck_parts, I get that this is a way to group deck part building tasks, but I don't really get the benefit of that. If anything, I see two downsides:

  • It looks like a transformer in its own right, but it's not... not really anyway. The real transformers are from_csvs, etc., right? I feel like having transformers at different nesting levels in the recipe file increases the cognitive complexity.
  • You mention that users would group all deck part building tasks at the top of the recipe, but what's the harm in interleaving them with generate tasks? Someone may want to have a recipe that builds a deck part, then does something with it, then builds another deck part and does something else with it. I don't know, I feel like it's just an unnecessary constraint.

I hope my reasoning makes more sense, but no worries if I didn't manage to convince you 😉. Thank you for taking the time to chat about this! 🍻

from brain-brew.

ohare93 avatar ohare93 commented on May 30, 2024

Further thought: perhaps each top level step should be explicitly named by the user 🤔 rather than it being a list. This would help with the end goal of allowing others to more easily replace one specific part of the Recipe. Rather than splitting up the steps into multiple files which is annoying and not as easily readable, or replacing a specific numbered step which would be prone to errors.

This could also just be optional, for repos such as UG which will actually use it, and the rest can use just a standard list.

Thoughts? 😁 This is the time for these high level design discussions, while everything is young and easily changed 😆

from brain-brew.

axelboc avatar axelboc commented on May 30, 2024

Just catching up with this. Awesome! 🍻 Glad to hear also that your code is already suited to making this change, I was worried it'd be too much work.

Further thought: perhaps each top level step should be explicitly named by the user 🤔 rather than it being a list.

Could you just please clarify what you mean with a quick example, to make sure I understand?

from brain-brew.

Related Issues (15)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.