Make known group labels require 'default' instead of 'en' #1316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This just makes it work the same as all configuration, which was changed a while ago. Both `TranslatedString` types were combined into one type. This commit also adjusts some docs that have been forgotten. There is a second subtle breaking change: it's only possible to import labels with languages that Tobira knows about, and not arbitrary two- letter codes, like before. I don't expect that to be a problem. Tobira was never able to actually show these in the UI anyway.
owi92
approved these changes
Jan 17, 2025
owi92
added a commit
that referenced
this pull request
Jan 20, 2025
This is the start of some backend refactoring. We noticed for a while that many types don't cleanly fit into one submodule like `db`, `api`, or something else, because they have DB impls, are loaded from the DB, are exposed in the API and potentially other uses. Having a new top level module house these make sense to me. I also don't mind having smaller files in there, with fewer types per file. And I also don't mind having many submodules inside `module`. Note that this doesn't mean that we won't have two types for the same "thing" at all anymore. Sometimes it makes sense to have a separate type! For example, see some `search::*` types below which represent exactly what's stored in the search index. We want to disconnect that from the API representation somewhat, they don't necessarily need to match. This PR does not perform the full refactor, far from it! It just adds the module and moves 3 interesting types already. To reduce merge conflicts I expect us to move things around over the next couple months. This is based on #1316, so that should be merged before. --- I went through all modules (except `api`) and checked all types. Here are more things I would move in the future: - Pretty much everything in `db::types` - `search::{TextSearchIndex, TextAssetType, TextMatch}` probably into some `model::text`? - `search::{Event, Playlist, Realm, Series, User}`: not sure about that. These are mostly a representation of whats stored in the search index and have a separate `SearchFoo` type in `api::model`. I think these can stay. With one exception: `search::Playlist` currently does not have a separate `SearchPlaylist` type. It should probably get one at some point. - Lots/most/all things in `api::model`? I haven't really looked at it closely yet. --- In terms of review: ehm... all the moving around and import changing is annoying to review and I don't really see the point. Maybe roughly look at the new structure? And maybe test a bit? Dunno, or nothing.
LukasKalbertodt
added a commit
to LukasKalbertodt/tobira
that referenced
this pull request
Feb 5, 2025
Things I forgot in elan-ev#1316. The reason for why I didn't notice is that the `TranslatedLabel` type was bad. In particular, `Record<string, _>` seems to behave differently than I assumed. It's very lenient. Now I made typing more strict in multiple places. In the backend, the translated string will also check the default field when loading from DB, in order to keep the API description guarantees. This fixes the ACL editor being buggy or crashing when dealing with users.
owi92
added a commit
that referenced
this pull request
Feb 5, 2025
Things I forgot in #1316. The reason for why I didn't notice is that the `TranslatedLabel` type was bad. In particular, `Record<string, _>` seems to behave differently than I assumed. It's very lenient. Now I made typing more strict in multiple places. In the backend, the translated string will also check the default field when loading from DB, in order to keep the API description guarantees. This fixes the ACL editor being buggy or crashing when dealing with users.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This just makes it work the same as all configuration, which was changed a while ago. Both
TranslatedStringtypes were combined into one type. This commit also adjusts some docs that have been forgotten.There is a second subtle breaking change: it's only possible to import labels with languages that Tobira knows about, and not arbitrary two- letter codes, like before. I don't expect that to be a problem. Tobira was never able to actually show these in the UI anyway.