#15 json naming convention #56
Conversation
…ing the file, so that the user can check whether it exists. This was useful for me, for example, for noticing that, although the json file existed, it was one folder deeper in the github repository than the plugin expected.
… jsons be called deck.json. This way their name doesn't need to be tied to the deck name or to the folder name or to the github repo name. Did some tests and it seems to work just fine. A repo called 'myRepo', when imported, will create a deck called 'Spanish castellano' from a json called 'deck.json'
|
This, makes sense, but I'd like to ensure backward compatibility on import. I.e. the import with the old convention should still work. |
crowd_anki/export/anki_exporter.py
Outdated
| @@ -28,7 +28,7 @@ def export_to_directory(self, deck: AnkiDeck, output_dir=Path("."), copy_media=T | |||
| deck = deck_initializer.from_collection(self.collection, deck.name) | |||
| self.last_exported_count = deck.get_note_count() | |||
|
|
|||
| deck_filename = deck_directory.joinpath(deck_fsname).with_suffix(DECK_FILE_EXTENSION) | |||
| deck_filename = deck_directory.joinpath('deck').with_suffix(DECK_FILE_EXTENSION) | |||
There was a problem hiding this comment.
Please extract the actual name used to a constant in https://github.com/Stvad/CrowdAnki/blob/master/crowd_anki/utils/constants.py
There was a problem hiding this comment.
good point, done in the latest commit.
crowd_anki/importer/anki_importer.py
Outdated
| @@ -42,7 +42,7 @@ def load_from_directory(self, directory_path, import_media=True): | |||
| aqt.mw.progress.start(immediate=True) | |||
|
|
|||
| try: | |||
| self.load_from_file(directory_path.joinpath(directory_path.name).with_suffix(DECK_FILE_EXTENSION)) | |||
| self.load_from_file(directory_path.joinpath('deck').with_suffix(DECK_FILE_EXTENSION)) | |||
There was a problem hiding this comment.
You should update the docstring of the function to describe new convention
There was a problem hiding this comment.
done in the latest commit.
|
Btw, I see several merge commits on the branch - please look into using the |
|
Also, the Readme should be updated to reflect new convention |
|
Apologies for the delay. I will look at implementing your suggestions next weekend. |
…kwards compatibility in the importer, and changed the function description
I've implemented this in the latest commit (and tested it too, locally; seems to work just fine). |
I've checked the Readme, and the only places that the file naming convention comes up seems to be in the screenshots. And I haven't changed them, partly because I didn't want my git username and projects to replace yours; it only makes sense that they reflect your workspace, given that you are the author of the plugin, at least that's what I reasoned. But let me know if you'd like these updated too, and I can do that. |
|
Hey, sorry for the delay in looking at this. I've checked it and merged it with minor updates |
|
Great, thank you!
…On Tue, Jul 30, 2019, 06:06 Vladyslav Sitalo ***@***.***> wrote:
Hey, sorry for the delay in looking at this. I've checked it and merged it
with minor updates
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#56?email_source=notifications&email_token=AANWGJTP7IUVNNAQUNVNB2LQB6445A5CNFSM4HHDW7L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3CWEPQ#issuecomment-516252222>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANWGJSUZQ6TOXJC3RYL4CLQB6445ANCNFSM4HHDW7LQ>
.
|
I'm suggesting a naming convention, where all the json files are named 'deck.json', which can simplify naming issues especially for the importer. Then it's both the folder they're in and also the github repo that offer the user the knowledge of which deck they have there.