Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow im-/exporting with or without deck configs #2804

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

RumovZ
Copy link
Collaborator

@RumovZ RumovZ commented Nov 4, 2023

Closes #2777.

@RumovZ
Copy link
Collaborator Author

RumovZ commented Nov 4, 2023

Just realized that we only import new configs. Should I implement updating existing ones? Otherwise, we should probably at least mention that limitation in the help string.

@dae
Copy link
Member

dae commented Nov 5, 2023

Thanks Rumo, I'll circle back to this after 23.10.1 is out the door. Let's skip updating unless someone makes a strong case for it, as I imagine it will cause people to think Anki is at fault when they update a deck and suddenly they're getting things like a different number of new cards a day.

@RumovZ
Copy link
Collaborator Author

RumovZ commented Nov 5, 2023

New cards per day may already change, though, because the deck itself is getting updated (things like NormalDeck.new_limit).

@dae
Copy link
Member

dae commented Nov 5, 2023

Don't we handle that in prepare_deck()?

@dae
Copy link
Member

dae commented Nov 5, 2023

(though we still need to de-couple 'contains scheduling' and 'include scheduling' - can't remember if I previously pointed ceb8a4a out to you)

@dae
Copy link
Member

dae commented Nov 5, 2023

Ah, just skimmed through your changes (hadn't looked at them yet) - I see you're now doing that based on include_config. Would it make sense to revert that part to being based on scheduling, and make a rule that deck authors can only adjust the limits in their template, and not on individual decks? We could think of the option as 'include preset' rather than 'include config' to make it clearer, and it would ensure we don't accidentally import the deck's current review count for the day if the exporter included scheduling and the importer didn't. It would also make it easier for importers to override the default daily limits if we implemented updating in the future.

@RumovZ
Copy link
Collaborator Author

RumovZ commented Nov 7, 2023

revert that part to being based on scheduling, and make a rule that deck authors can only adjust the limits in their template, and not on individual decks

I was also considering that, but couldn't decide and opted for the (slightly) simpler alternative. I will change it.

ensure we don't accidentally import the deck's current review count for the day if the exporter included scheduling and the importer didn't.

Funny you mention it. I see I've accidentally dropped the reset of deck.common. 😅

@RumovZ RumovZ marked this pull request as draft November 7, 2023 17:21
Also:
- Fix `deck.common` not being reset.
- Apply all logic only depending on the source collection in the
gathering stage.
- Skip checking for scheduling and only act based on whether the call
wants scheduling. Preservation of filtered decks also depends on all
original decks being included.
- Fix check_ids() not covering revlog.
@RumovZ RumovZ force-pushed the import-deck-configs branch from 8087916 to 6639657 Compare November 9, 2023 20:59
@RumovZ RumovZ marked this pull request as ready for review November 10, 2023 16:01
@dae
Copy link
Member

dae commented Nov 13, 2023

Thanks Rumo!

@dae dae merged commit f200d62 into ankitects:main Nov 13, 2023
with_scheduling: bool,
with_media: bool,
legacy_support: bool,
self, *, out_path: str, options: ExportAnkiPackageOptions, limit: ExportLimit
) -> int:
Copy link
Member

@dae dae Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(musing to myself: this is an API break. Given that, and we're adding new features, this next update might be better called 23.12 than 23.10.2)

@RumovZ RumovZ deleted the import-deck-configs branch November 13, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider an option to export/import deck preset values
2 participants