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

refactor: Sub-class Session from db.Entity to behave dict-compliant #1153

Merged
merged 16 commits into from
May 28, 2024

Conversation

KadirBalku
Copy link

@KadirBalku KadirBalku commented May 6, 2024

Resolves #1147, replaces #1140.

Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @KadirBalku, thanks for your first pull request!

I've added some improvement suggestions.
Can you test your branch with other projects than "viur-trial" please, just for confidence.

src/viur/core/session.py Outdated Show resolved Hide resolved
src/viur/core/session.py Outdated Show resolved Hide resolved
@phorward phorward added the refactoring Pull requests that refactor code but do not change its behavior. label May 6, 2024
@phorward phorward added this to the ViUR-core v3.7 milestone May 6, 2024
@phorward phorward added the Priority: Medium This issue may be useful, and needs some attention. label May 6, 2024
@phorward phorward changed the title feat: Session is now subclassed from db.Entity refactor: Sub-class Session from db.Entity to behave dict-compliant May 6, 2024
KadirBalku and others added 2 commits May 6, 2024 16:54
Co-authored-by: Jan Max Meyer <jmm@phorward.de>
Co-authored-by: Jan Max Meyer <jmm@phorward.de>
@KadirBalku KadirBalku requested a review from phorward May 6, 2024 14:55
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @KadirBalku,

have you tested it with other projects?

And can you please take a look at the functions I've commented. Maybe they must be kept, or the internally use __setitem__. In latter case, all is fine, and the changed-flag is being set properly.

Take a look into the Python docs to get an overview about all dict-related functions.

src/viur/core/session.py Show resolved Hide resolved
src/viur/core/session.py Show resolved Hide resolved
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

@KadirBalku looks good to me!
Please fix the unused import issue, then we can merge.

src/viur/core/session.py Outdated Show resolved Hide resolved
Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

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

What about Session.pop() and where is Session.setdefault() from #1140?

src/viur/core/session.py Outdated Show resolved Hide resolved
src/viur/core/session.py Outdated Show resolved Hide resolved
@phorward
Copy link
Member

What about Session.pop() and where is Session.setdefault() from #1140?

@KadirBalku Session.pop() must be implemented.
@sveneberth Session.setdefault() is provided by sub-classing db.Entity, which sub-classes dict.

KadirBalku and others added 2 commits May 17, 2024 14:22
In the viur-meeting on 2024-04-12, it was decided to change the
licensing terms of the viur-core component from the LGPL-v3 into the MIT
license.

This pull request updates all relevant parts of the codebase.

---------

Co-authored-by: Sven Eberth <mail@sveneberth.de>
src/viur/core/session.py Outdated Show resolved Hide resolved
src/viur/core/session.py Outdated Show resolved Hide resolved
@phorward phorward added the viur-meeting Issues to discuss in the next ViUR meeting label May 21, 2024
@phorward
Copy link
Member

@KadirBalku we should discuss about dropping Python 3.10 support with viur-core>=3.7

Co-authored-by: Sven Eberth <mail@sveneberth.de>
Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

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

LGTM, But we should discuss the python 3.10 issue first, before we merge this. So let's wait until Friday.

@sveneberth
Copy link
Member

I removed python3.10 support as decided in the viur-meeting today. After we merged that we can merge this pr whithout a failing pipeline

@phorward phorward removed the viur-meeting Issues to discuss in the next ViUR meeting label May 24, 2024
@phorward phorward self-requested a review May 27, 2024 16:14
@phorward phorward requested a review from sveneberth May 27, 2024 16:15
@phorward phorward merged commit 56f110b into viur-framework:develop May 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. refactoring Pull requests that refactor code but do not change its behavior.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

None yet

4 participants