Skip to content

Comments

Public links ORMified#18

Merged
jessegeens merged 6 commits intomasterfrom
feat/public-links-ormified
Mar 3, 2025
Merged

Public links ORMified#18
jessegeens merged 6 commits intomasterfrom
feat/public-links-ormified

Conversation

@jessegeens
Copy link
Contributor

The code managing public links was seperated from the code for managing shares up to now. With this PR we change this, and we put the two together.

This was necessary because the old code, which is stored in Reva, still uses the old oc_share table, which means it is not compatible with our new ORMified schemas

@jessegeens jessegeens marked this pull request as draft February 25, 2025 13:08
@jessegeens jessegeens force-pushed the feat/public-links-ormified branch from 2f21450 to 412e47f Compare February 25, 2025 13:16
@jessegeens jessegeens marked this pull request as ready for review February 25, 2025 13:21
@jessegeens jessegeens requested a review from diocas February 25, 2025 13:36
@jessegeens jessegeens force-pushed the feat/public-links-ormified branch 6 times, most recently from 4d2cadf to 97d0eed Compare February 26, 2025 13:39
@jessegeens jessegeens force-pushed the feat/public-links-ormified branch from 97d0eed to c863a19 Compare February 27, 2025 09:18
@jessegeens jessegeens force-pushed the feat/public-links-ormified branch from 93f32e3 to 641d885 Compare February 27, 2025 10:11
Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

We are changing the name of this managers to be called "gorm", right? So I would make this uniform between reva(?)shares and publicshares. An change the folder name.

I would also avoid some duplicate code related to DB opening and such. You can split it and re-use in 2 different places. But the split you did to not have publicshares and reva(?)shares mixed in shares.go is already better.

@jessegeens
Copy link
Contributor Author

We are changing the name of this managers to be called "gorm", right?

Only for public links, because there is already a sql driver for public links (in reva). I agree that it's a bit inconsistent now, but instead of renaming the sharing driver to gorm too, I am more a fan of ripping out the old public links sql driver from Reva and renaming this one to be a sql driver too, to make it consistent with the rest of Reva

@jessegeens jessegeens force-pushed the feat/public-links-ormified branch 2 times, most recently from 4a1d544 to d94f178 Compare February 28, 2025 08:42
@jessegeens jessegeens requested a review from diocas February 28, 2025 08:49
@diocas
Copy link
Contributor

diocas commented Feb 28, 2025

I am more a fan of ripping out the old public links sql driver from Reva and renaming this one to be a sql driver too

I totally agree. If we can do it already, let's do it. If not, making it consistent would be better for now.

@jessegeens
Copy link
Contributor Author

I am more a fan of ripping out the old public links sql driver from Reva and renaming this one to be a sql driver too

I totally agree. If we can do it already, let's do it. If not, making it consistent would be better for now.

I guess we can already. I will modify the driver here to be called sql then. Will then make a Reva PR; and we can merge this one once the Reva PR is ready

@jessegeens jessegeens force-pushed the feat/public-links-ormified branch from d94f178 to 3a73693 Compare February 28, 2025 16:17
Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

Last sprint!

Copy link
Contributor

@diocas diocas left a comment

Choose a reason for hiding this comment

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

I promise is the last one...

@jessegeens jessegeens force-pushed the feat/public-links-ormified branch from 15f0567 to 661e4e6 Compare March 3, 2025 09:12
@jessegeens jessegeens requested a review from diocas March 3, 2025 09:12
@jessegeens jessegeens merged commit d4c5866 into master Mar 3, 2025
1 check passed
@diocas diocas deleted the feat/public-links-ormified branch March 3, 2025 09:42
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.

2 participants