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

vval - technical onboarding #623

Open
wants to merge 2 commits into
base: 18.0
Choose a base branch
from

Conversation

vval-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Mar 17, 2025

Pull request status dashboard

Copy link

@dbeguin dbeguin left a comment

Choose a reason for hiding this comment

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

Good work already :)
some picky comments mainly regarding guidelines :)

@vval-odoo vval-odoo force-pushed the 18.0-realEstate-module-vval branch 7 times, most recently from bec23fa to c769eb4 Compare March 20, 2025 10:33
Module to handle real estate properties and salesflow.
This module allows to create invoices directly from the estate property form when the property has been marked as 'sold'.
@vval-odoo vval-odoo force-pushed the 18.0-realEstate-module-vval branch from ed3d58f to d18afcc Compare March 20, 2025 16:08
Copy link

@dbeguin dbeguin left a comment

Choose a reason for hiding this comment

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

Zblip ! A set of remark for code polishing and commit message beautifulness
Enjoy!

@@ -0,0 +1 @@
from . import models
Copy link

Choose a reason for hiding this comment

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

Commit message:
if you use [ADD], which is the right thing to do in this case, you already say that you add a module, so no need to make it your commit title. Explain what the module does in few words, beginning by a verb.
"[ADD] estate: handle real estate properties and salesflow"
Then in description, you can describe what features and capabilities the module have
and even list all the models used to handle the business cases. What do what and how they interact.
Basically, when reading commit message, you don't need to read the code to know how the module works. Or reading commit message will help you understand the code.
Better reading 10 lines of real words than 250 lines of code.
The goal of the commit message is mainly to help future debugging or refactoring or functionality rework, when you try to find why a certain line has been added to the code base, you read the commit message to get the context in which the commit content has been added. Knowledge is power. Better too much info than not enough :)

@@ -0,0 +1 @@
from . import models
Copy link

Choose a reason for hiding this comment

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

Commit message:
[ADD] estate_account: allows to create invoices for 'sold' properties
then add details

@@ -0,0 +1,4 @@
{
Copy link

Choose a reason for hiding this comment

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

You miss the version, summary, description, category, application=False, installable=False (because automatically installed when both accounting and estate module will be installed)
author and license.
apply this in the other commit as well

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.

3 participants