-
Notifications
You must be signed in to change notification settings - Fork 81
[ADD] util/report: util.add_report #360
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
base: master
Are you sure you want to change the base?
Conversation
cc49acd to
7dae849
Compare
|
upgradeci retry with always only crm |
aj-fuentes
left a comment
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.
I think we can do some simplification here. Some suggestions below. Note that none of the suggestions were tested, take it as a guide.
b40852a to
3525d5d
Compare
3525d5d to
7b3739f
Compare
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.
Looks better. Please adapt the documentation to the same format we use elsewhere in this repo. Some suggestions above.
c48c6f9 to
64d430b
Compare
| return "<li>{}</li>".format(row_format.format(**row_dict)) | ||
|
|
||
| if not data: | ||
| row_to_html(columns) # Validate the format is correct, including links |
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 will fail. column doesn't have the expected type.
(a.k.a tests are missing)
src/util/report.py
Outdated
| _logger.warning("Upgrade report is growing suspiciously long: %s characters so far.", migration_reports_length) | ||
|
|
||
|
|
||
| report = add_to_migration_reports |
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.
If we want to promote the usage of this new function, I would rename the function, so it appears with the good name in documentation, and alias the old name for compatibility.
| report = add_to_migration_reports | |
| add_to_migration_reports = report |
4f10c12 to
e48f6a3
Compare
Add a new function that replaces util.add_to_migration_reports. The new function can automatically parse the provided data as a list of records. It can also limit the number of displayed records to prevent the upgrade report from growing too large. The structure of the note is now defined within the function itself, standardizing its appearance.
e48f6a3 to
1bae825
Compare
| 100, | ||
| "Other", |
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.
I had to pass the default arguments as regular ones, because of how the tested method is called in test_report_with_list. Simply leaving them out would obviously result in
Traceback (most recent call last):
File "/home/odoo/src/upgrade-util/src/testing.py", line 174, in wrapped
return func(self, *args)
TypeError: TestReportUtils.test_report_with_list() missing 2 required positional arguments: 'category' and 'expected'
Passing them as None also wouldn't work, because since Python differentiates between "no argument passed" and None, they would actually be set to None instead of taking the default value. And if we pass category=None, then add_to_migration_reports will break on running len(None).

Add a new function that replaces
util.add_to_migration_reports.The new function can automatically parse the provided data as a list of records. It can also limit the number of displayed records to prevent the upgrade report from growing too large.
The structure of the note is now defined within the function itself, standardizing its appearance.
Example usage
More examples will follow in
odoo/upgrade, replacing the existing use ofutil.add_to_migration_reports.Example 1
Example 2
Example 3
Example 4
Remaining details to discuss
limit.columns(instead of a tuple) or a dictionary of lists forlinks(instead of a dictionary of tuples).add_to_migration_reports.ir.ui.viewrecord corresponding to the note is sometimes incorrect (previously there was the same issue):