-
-
Notifications
You must be signed in to change notification settings - Fork 672
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
[IMP] Migration of base_custom_attributes #59
Conversation
Hey @fevxie, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
Hi @fevxie
this should make the build green. |
Hi, @gurneyalex, I'm working for Elico Corp currently who signed the Enterprise CLA and I also will signed my personal CLA and send email to community. Thanks for your work. |
Hey @fevxie, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
1 similar comment
@fevxie I updated your information in the OCA database, and you should no longer get flagged by the CLA bot. Thanks for your contributions. |
1 similar comment
what should I do for this PR? |
A module add custom attribute for models | ||
=========================================== | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small description would be great
@fevxie Your 3 first commits need fixing with author. You know what to do 😉 |
return [(model.model, model.name) for model in models] | ||
|
||
name = fields.Char( | ||
size=128, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this size limit necessary?
@api.model | ||
def _get_default_model(self): | ||
context = self._context | ||
if context and context.get('force_model'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is enough to write
if context.get('force_model'):
as context is always a frozen dict even if empty
@fevxie See my few remarks About char size limit, this has no meaning for database but is a constraint when filling the field |
@yvaucher Thanks for you patient review. I will fix it ASAP. |
It seems checks does not fully test the code. At this point, we need to figures out what wrong with the code, the checks or the tests.
Ok I see that you fixed that. Maybe you want to squash those 3 last commits in one. Otherwise 👍 |
@@ -0,0 +1,28 @@ | |||
Base Custom Attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add AGPL icon see template :
https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst
Any update on this? |
No, this module is not going to be finally migrated. Check base_custom_info and product_custom_info instead. |
I've already checked these modules and they don't provide different field types for attribute values as base_custom_attributes did, only char. |
Initial migration base_custom_attribute, Still work in progress for fully test