Onboarding Server framework 101#1280
Conversation
c0d88d0 to
f310ea3
Compare
f310ea3 to
4bb98e2
Compare
Megaaaaaa
left a comment
There was a problem hiding this comment.
Hello 👋
Here's a first review for you, that's already a really nice start 👍
There's quite a lot of stuff but don't worry about it, it's a lot of the same small things that repeat themselves.
When receiving a review, you can go through all comments and apply what is suggested or bring your own alternative. Keep in mind that a review is not the utlimate solution, it's only someone seeing things he would have done differently and offering an alternative. You're always free to answer with an other alternative or even say you disagree and bring your arguments.
What I would suggest you do and continue to do while you're not the most comfortable with odoo's structure is to not apply all the changes at once. For example, changing a model's _name has a lot of impact and if you change everything at once, you might end up with a lot of errors when running the database. Splitting the review in multiple steps and trying to run your db in between those can save you a lot of time in the long run.
Also a quick tip for the methodology when applying reviews changes: What I like to do it put a reaction like a thumbsup on a message once I have made the change locally. Then when everything is marked with something, I push, go through my diff and mark as resolved the things that are now outdated so nothing is left forgotten.
If you have any question, don't hesitate to ask!
| from . import estate_property | ||
| from . import estate_property_type | ||
| from . import estate_property_tag | ||
| from . import estate_property_offer No newline at end of file |
There was a problem hiding this comment.
Every file must end with a line break so that if someone adds something after your last line, he doesn't alter the history of your line by adding it 👍
| from . import estate_property_offer | |
| from . import estate_property_offer | |
This is also known as the "EOL", the End Of Line character, I'll only tell you "EOL" for the other occurences 😄
| @@ -0,0 +1,5 @@ | |||
| # flake8: noqa: F401 | |||
| from . import estate_property | ||
| from . import estate_property_type | ||
| from . import estate_property_tag | ||
| from . import estate_property_offer No newline at end of file |
There was a problem hiding this comment.
Depending on the team you join, they might ask you to order your imports alphabetically so you might aswell start now, just in case (it's highly probable tbh) 😄
| from . import estate_property | |
| from . import estate_property_type | |
| from . import estate_property_tag | |
| from . import estate_property_offer | |
| from . import estate_property | |
| from . import estate_property_offer | |
| from . import estate_property_tag | |
| from . import estate_property_type |
| from odoo import models, fields, api, exceptions | ||
| from odoo.tools import float_compare, float_is_zero | ||
| from datetime import date | ||
| from dateutil.relativedelta import relativedelta |
There was a problem hiding this comment.
Every single team will ask to do this one: order the imports. Modules alphabetically and attributes asciibetically. It's clearer in the coding guidelines with an example 😄
| from odoo import models, fields, api, exceptions | |
| from odoo.tools import float_compare, float_is_zero | |
| from datetime import date | |
| from dateutil.relativedelta import relativedelta | |
| from datetime import date | |
| from dateutil.relativedelta import relativedelta | |
| from odoo import api, exceptions, fields, models | |
| from odoo.tools import float_compare, float_is_zero |
| from dateutil.relativedelta import relativedelta | ||
|
|
||
|
|
||
| def three_months_from_now(_model): |
There was a problem hiding this comment.
I think we would more often see:
| def three_months_from_now(_model): | |
| def three_months_from_now(self): |
even if you don't use self in the method. But what we would see evne more is this being done in the field declaration directly
date_availability = fields.Date(copy=False, default=lambda x: date.today() + relativedelta(months=3))It's up to you 👍
| 'name': "Real Estate", | ||
| 'version': '1.0', | ||
| 'depends': ['base'], | ||
| 'author': "Odoo S.A. (aykar)", |
There was a problem hiding this comment.
As an odoo employee you cannot sign modules in your own name, only in Odoo's
| 'author': "Odoo S.A. (aykar)", | |
| 'author': "Odoo S.A.", |
| 'installable': True, | ||
| 'application': True, | ||
| 'liscence': 'LGPL-3', | ||
| # data files always loaded at installation |
There was a problem hiding this comment.
| # data files always loaded at installation |
| 'views/estate_property_offers_views.xml', | ||
| 'views/estate_menus.xml', | ||
| ], | ||
| # data files containing optionally loaded demonstration data |
There was a problem hiding this comment.
| # data files containing optionally loaded demonstration data |
| ], | ||
| # data files containing optionally loaded demonstration data | ||
| 'demo': [ | ||
| # 'demo/demo_data.xml', |
There was a problem hiding this comment.
I guess this will be useful later but becareful to not push fragments of features because you might end up forgetting them 👍
| """, | ||
| 'installable': True, | ||
| 'application': True, | ||
| 'liscence': 'LGPL-3', |
There was a problem hiding this comment.
small typo
| 'liscence': 'LGPL-3', | |
| 'licence': 'LGPL-3', |

No description provided.