I have wrapped things in the Card component in the pages I have created, for example, the Admin Logs Page and the Relationship Types page. I'm aware that we only used Cards for the entity editor, and not any of the pages. If it is something which might lead to non-uniformity, then I can remove the card component and make it look more like the Revisions page or the Collections page. Personally, I like the header the Card
2023-08-01 21327, 2023
ShivamAwasthi
component provides for the title of the page, that is why I get naturally inclined to use it
2023-08-01 21316, 2023
ShivamAwasthi
Although I dont think we really have any strict uniformity in place, with respect to how it should look depending on whether its a form or a display page. Because for example, the Collection creation form does not use the Card component.
2023-08-01 21329, 2023
BrainzGit has quit
2023-08-01 21347, 2023
BrainzGit joined the channel
2023-08-01 21318, 2023
kellnerd
monkey: Wow, it looks like yesterdays type issue with the aliases is indeed unsolvable because it hints that there is a real *bug* in the existing code 🤯
Here we are passing an array of aliases without 'id' props but the function expects aliases with IDs.
2023-08-01 21314, 2023
kellnerd
If I understood correctly, this means that this function should only be called with aliases which were already inserted into the DB and hence have an ID.
2023-08-01 21301, 2023
kellnerd
But inside the createImport function we are only trying to create an AliasSet, not the aliases themselves!
2023-08-01 21301, 2023
kellnerd
I'm a bit shocked to be honest, because I had assumed that this code had been tested in 2018, I think I've even seen screenshots of the importer UI with pending imports!?
2023-08-01 21313, 2023
kellnerd
Does that mean I cannot trust any of the existing code?
I don't know if any of this code was tested. It's from just before my time working on BookBrainz
2023-08-01 21350, 2023
monkey
Interesting
2023-08-01 21329, 2023
kellnerd
(Not that the time of failure made a difference, since all actions are inside a transaction which got rolled back every time)
2023-08-01 21310, 2023
kellnerd
Ok, so we basically have to assume that the whole importer is untested from now on...
2023-08-01 21356, 2023
monkey
> If I understood correctly, this means that this function should only be called with aliases which were already inserted into the DB and hence have an ID.
2023-08-01 21325, 2023
monkey
Just read the `updateAliasSet` function, it looks to me like it also expects aliases without ids.
2023-08-01 21339, 2023
monkey
It inserts new aliases in the database as well as creating a new set, if I read correctly
... where I'm currently unsure whether Insertable should remove 'id' or make it optional.
2023-08-01 21347, 2023
kellnerd
First question is whether Form* and Parsed* have any meaning besides their original use cases.
2023-08-01 21308, 2023
kellnerd
ParsedAlias = FormAliasWithDefaultT - Id basically, which lets me think we don't need either of Form* and Parsed* in the final type names.
2023-08-01 21348, 2023
kellnerd
Once we are using types in more places, they will probably useful in other areas of the codebase as well.
2023-08-01 21311, 2023
kellnerd
So my suggestion would be to start with a type named Alias or AliasT (useful if there is a class and/or React component of the same name) and everything which needs more or less (special) properties gets a longer type name.
2023-08-01 21313, 2023
kellnerd
I think it probably makes sense to strip the LazyLoaded part from the basic type, but I'm unsure whether 'id' should be part of the basic type or not.
2023-08-01 21356, 2023
monkey
Agree with a simple `AliasT` with basic types (i.e. without id) and extend from there. That's the sort of composability that will make it more useful in the other repos
2023-08-01 21316, 2023
monkey
> but I'm unsure whether 'id' should be part of the basic type or not.
2023-08-01 21351, 2023
monkey
I reckon it does make sense to remove it. We rarely use the id, as you pointed out in the alias set update code it's only used once for example
2023-08-01 21332, 2023
monkey
I think this is where ParsedAliasT comes from: Alias as parsed from the DB by the ORM, meaning for example it will have an id
2023-08-01 21357, 2023
monkey
The name is cryptic though.
2023-08-01 21355, 2023
kellnerd
ParsedAlias is my creation, it's a WIP type for the importer project :)
2023-08-01 21325, 2023
kellnerd
It's something which gets inserted into the import queue
2023-08-01 21300, 2023
kellnerd
But I agree, the name is not good since I didn't knew exactly how it relates to the ORM when I defined it.
2023-08-01 21309, 2023
monkey
Ahhh
2023-08-01 21323, 2023
monkey
OK, makes more sense now thanks
2023-08-01 21333, 2023
monkey
Not I think the name is OK in that context
2023-08-01 21315, 2023
kellnerd
Having AliasT defined without 'id' also has the nice side effect that "lazily" typed functions wouldn't require an 'id' attribute which they may not need (as it happened here).
2023-08-01 21327, 2023
monkey
So what would be an alias that was previously saved and retrieved from the DB, with the extra id attribute? Simply `AliasWithIdT` N
2023-08-01 21332, 2023
monkey
-N + ?
2023-08-01 21351, 2023
kellnerd
^ i.e. one would have to explicitly choose a type which has an 'id'
2023-08-01 21316, 2023
kellnerd
That or FetchedAliasT?
2023-08-01 21334, 2023
monkey
I think that's reasonable. Expect a basic Alias, but if your use case requires an alias with ID choose the right specific type.
2023-08-01 21351, 2023
monkey
🤔
2023-08-01 21310, 2023
monkey
Yeah, in case there's more than just an extra id field
2023-08-01 21334, 2023
kellnerd
Let me have a quick look what other libraries do...
2023-08-01 21341, 2023
monkey
The name is good, but also requires some context to understand. I don't have a better suggestion
Back on topic: I guess that I should start with refactoring the existing conflicting types, the only problem is that they are used across three different repositories.
2023-08-01 21334, 2023
kellnerd
A common problem which I have.
2023-08-01 21340, 2023
sl4n joined the channel
2023-08-01 21320, 2023
monkey
Ahhhh, brainfuck <3
2023-08-01 21329, 2023
monkey
I attempted to lear it a long time ago
2023-08-01 21341, 2023
monkey
I'd say refactor the types you need for now (which will be a lot of them!), we'll lukely need to completely reevaluate our approach when we change ORMs
2023-08-01 21313, 2023
kellnerd
I agree
2023-08-01 21327, 2023
kellnerd
Already having a decent type coverage also makes me feel more comfortable about doing a huge refactoring such as replacing the ORM.
