OnkelTem / anki-dm

Anki Deck Manager
39 stars 4 forks source link

Store name of field/template in JSON rather than filename #1

Closed axelboc closed 6 years ago

axelboc commented 6 years ago

I'm wondering whether it's a good idea to have the name of the fields and templates directly as filenames.

First, having capital letters in filenames may create issues on some case-insensitive systems. Second, I had an issue with importing the Ultimate Geography deck because its templates have a special character in their names : >, so the JSON files for the templates couldn't be created on my Windows machine.

Would it be possible to move the exact name of a field/template inside the content of its JSON file, and to use a kebab-case slug for the filename?

axelboc commented 6 years ago

P.S. Apart from that, the import worked like a charm!! ☺️

OnkelTem commented 6 years ago

Hi Axel!

It's nice to see you tried it!

Yeah, I expected this file-based naming could be a problem so my plan was to add proper escaping. I'll fix it asap.

As for storing names inside files, well, it's not hard to do this — and honestly this is how it worked at first, until I removed it. I just didn't want to duplicate any information, because it could bring problems with synchronization and updates.

For example to get the list of fields you now simple list files: ls src/fields -1. With fields inside you need to check the content of every file. Or if you need to rename 5 fields, now you just rename 5 files, but with field names inside - you have to update 5 files. Or, if a field is named differently inside and outside - which name is more "right"? Should we care about filenames at all then? Whose job is to keep them in sync? This was my reason basically.

What do you think, will it be enough to escape field names properly and update templates correspondingly?

axelboc commented 6 years ago

Yeah I get you, the duplication isn't great.

I just had a thought, though: do fields really need to be in separate files? Perhaps a file called fields.json at the root of the src folder would do just fine? It would bypass the filename issue, and you wouldn't even have to move the name property out of and into the JSON.

Of course, templates do need to stay in separate files, but their naming doesn't quite matter as much, so I might just go with renaming them to PascalCase and screw case-insensitivity! 😄

OnkelTem commented 6 years ago

I just had a thought, though: do fields really need to be in separate files? Perhaps a file called fields.json at the root of the src folder would do just fine? It would bypass the filename issue, and you wouldn't even have to move the name property out of and into the JSON.

Good point. I thought about it too. But could we go even further and remove fields completely? Sound too daring? Let's take a look at the fields structure:

{
  "font": "Arial",
  "media": [],
  "rtl": false,
  "size": 20,
  "sticky": false
}

With the exception of "media" property (of purpose of which I'm not sure at all) all others affect only edit form. If we're not going to edit decks inside Anki anymore we can safely remove this customization and provide defaults. So we have now just a list of fields right? But fields are already listed in the header of data.csv so... let's get rid of them? :)

axelboc commented 6 years ago

Oh really? I was wondering what these properties were for... Well, sounds good to me!

OnkelTem commented 6 years ago

Ok, it's done. Please check out this branch: https://github.com/OnkelTem/anki-dm/tree/remove-fields-definitions

List of changes:

P.S. Just in case, this is my composer.json:

{
  "name": "-",
  "description": "-",
  "repositories": [
    {
      "type": "vcs",
      "url": "https://github.com/OnkelTem/anki-dm.git"
    }
  ],
  "require": {
    "onkeltem/anki-dm": "dev-remove-fields-definitions"
  }
}
axelboc commented 6 years ago

Thanks! I won't have time to try it this week-end, but I will asap.

OnkelTem commented 6 years ago

@axelboc I've tested this new branch on my countries deck, it worked well, so I've merged the branch into master. No need to change composer.json.

axelboc commented 6 years ago

Ah great!