Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
18.0 tutorial drod #178
base: 18.0
Are you sure you want to change the base?
18.0 tutorial drod #178
Changes from 7 commits
c61bcc4
e7b73dc
e889b60
0f0cd1a
55bc134
9472a65
9d9e624
e749a1d
e2bbeeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Better to give each its own line: clearer and later, if some files have been added, people can find more easily who was at the origin of a specific file with a git blame
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.
We use
_
only in specific cases, in a model name. It is better to use a.
in between words (you could think of it as the model "property" of the module "estate" ->estate.property
).This is the same for all other model names
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.
Avoid unnecessary empty lines between fields declarations, it takes a lot of space and doesn't bring much clarity :D
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.
You could also use
fields.Date.today() + relativedelta(months=3)
if you wanted, it would use one less importThere 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.
Fields will always default to a "nothing" state (0 for integers and floats, False for boolean, '' for strings) so you don't have to specify the
default
parameter to set fields to those values ;)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.
Not wrong, but the parenthesis are not needed
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.
Better to use
float_compare
fromodoo.tools.float_utils
to avoid rounding errorsThere 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.
No need to
return True
for actions, but it won't create any issue, so 🤷♂️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.
Usually we prefer to put a model per file, unless their definition doesn't take much space... As it is the case here, not need to change anything 👍
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.
Try duplicating a property type: you'll have an issue due to the constraint. How can you resolve that ?
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.
Compute functions should always start with
_compute
;)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.
Same duplication error as for the property type, but as this model doesn't have a lot to duplicate, the feature is less used so the problem is less important (but still)...
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.
This is unnecessary, we just want the tags on the properties, but we don't really care to find properties from a tag (a search by tags on the properties list view will suffice if we want to find properties with a special tag