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: BaseInvenTreeSetting #4834

Merged
merged 7 commits into from
May 18, 2023

Conversation

wolflu05
Copy link
Contributor

@wolflu05 wolflu05 commented May 16, 2023

This PR tries to add typings for settings and make the common.BaseInvenTreeSetting model more generic. This simplifies the implementation for the #4824 , see #4824 (comment)

Actually first, I planned to only add a get_filters method like the get_kwargs method until I now discovered that something like this was already there onetime and was removed. (See: 5ee9af7). Let me know if you have a better idea to make it more generic.

I tested this and discovered no bugs or issues that something doesnt work or break with this PR. Hopefully tests cover the other half 😄.

@matmair
Copy link
Member

matmair commented May 16, 2023

@wolflu05 are you sure that the subclass checking is not required for plugin settings?

Edit: It seems there is no coverage on the changed settings code - that would need to be changed

Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

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

Looks good in general

InvenTree/plugin/models.py Show resolved Hide resolved
@wolflu05
Copy link
Contributor Author

are you sure that the subclass checking is not required for plugin settings?

I used project wide code search and checked all method calls individually where I changed something if there is a plugin kwarg. And then looked through the list for plugin= results, changed everything there what I found to call the .plugin_config() function there.

@matmair
Copy link
Member

matmair commented May 16, 2023

So we can remove that section entirely?

@wolflu05
Copy link
Contributor Author

What section? The subclass checking in the BaseInventreeSettingClass? Yes. Ideally everything is moved out now and should be implemented in new code like this.

Do you want me to add general tests in this PR? But how do we test an abstract model?

@wolflu05
Copy link
Contributor Author

wolflu05 commented May 17, 2023

How do I interpret the coverage report? Especially this line? Should there something be improved? And where is the coverage decrease in this file?

@wolflu05 wolflu05 mentioned this pull request May 17, 2023
@matmair
Copy link
Member

matmair commented May 17, 2023

Something seems off with the coverage - I will let it run again

Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

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

Looks good but not sure if the uniqueness will work this way

@wolflu05
Copy link
Contributor Author

How do you mean that?

@matmair
Copy link
Member

matmair commented May 18, 2023

How do you mean that?

I was worried about removing the kwargs passed to the uniqueness check for user settings, in local testing it seems to work this way though.

@matmair matmair added this to the 0.12.0 milestone May 18, 2023
@matmair
Copy link
Member

matmair commented May 18, 2023

@SchrodingersGat 0.12.0 is looking like a big change under the hood - I think especially this one needs a bit of testing in the real world before we release a stable

@SchrodingersGat SchrodingersGat merged commit cb8ae10 into inventree:master May 18, 2023
@SchrodingersGat
Copy link
Member

@wolflu05 thanks for the contribution

@matmair agreed, we won't be pushing out this release immediately!

@wolflu05 wolflu05 mentioned this pull request May 30, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants