[IMP] upgrade_analysis: generate coverage module file

[legalsylvain]

Generate coverage file automatically. see : OCA/OpenUpgrade#2964

• todo sort the module list.
• try to change for qweb templating. Tried. Not fun.

CC : @pedrobaeza, @StefanRijnhart

[pedrobaeza]

Good idea! I would prefer though to not use a new rendering templating system. I think you can do it by QWeb, isn't it?

[rvalyi]

Good idea! I would prefer though to not use a new rendering templating system. I think you can do it by QWeb, isn't it?

Can you generate non XMlL files and control rst lines and spaces properly with qweb? I'm not sure but may be.

[pedrobaeza]

Can you generate non XMlL files and control rat lines and spaces properly with qweb? I'm not sure but may be.

AFAIK, you can generate what you want. Just use <t> directives instead of other HTML tags.

[legalsylvain]

Thanks all for your reviews !

@pedrobaeza said here.

The sorting is important though

Fixed. I added a sorted.

@StefanRijnhart said here

Can we prevent upgrade_analysis and other non-core modules from showing up here?

Well, I'd like to keep the possibility to have an agnostic module ugrapde_analysis that can be used to intialize openupgrade project (of course) but also to analyze custom modules changes.
i added a domain, to have only installed module. (and also excluding upgrade_analysis module because it doesn't make sense)
I think it should be good, now.

I updated the file here : OCA/OpenUpgrade#2964

Could you take a look ?

Good idea! I would prefer though to not use a new rendering templating system. I think you can do it by QWeb, isn't it?

Never tried. allways saw html / xml generated via qweb. I'll take a look.

[StefanRijnhart]

@legalsylvain current result is fine, thanks.

[StefanRijnhart]

Oh wait, can you compute the column width in an earlier pass to prevent this

+--------------------------------------------+-------------------------------------------------+
| |add| purchase_requisition_stock_dropshipping|                                                 |
+--------------------------------------------+-------------------------------------------------+

[legalsylvain]

AFAIK, you can generate what you want. Just use directives instead of other HTML tags.

@pedrobaeza,

Indeed. However, I just tried during 1 hour to use qweb to generate text. It's a mess to handle correctly return lines (not having a lot of \n), and the temporary result is ugly, less readable and very more verbose as the mako one.

As this module doesn't have to be installed on any production, and it's just for technical purpose (openupgrade analysis or custom analysis), I propose to to let the mako dependency. If it's a problem, we can add it in the roadmap section if you want.

Oh wait, can you compute the column width in an earlier pass to prevent this

@StefanRijnhart,
I increased the size of the column, and it is now OK. (dynamically calculating the size of the column makes the template less readable).

• Travis is green on the current PR.
• I updated the result and it seems OK. [ADD] main coverage file OpenUpgrade#2964

Hope all is OK for you.

[pedrobaeza]

Isn't t-raw + html code (
) for line feed enough?

[legalsylvain]

Isn't t-raw + html code ( ) for line feed enough?

I don't know but unfortunately i have no time to investigate.
For me that PR is a good step forward. (before it was a custom script executed on some openupgrader computer, now it is integrated in the main upgrade analysis module)

[legalsylvain]

I added some improvments

• If ---nothing has changed in this module-- is in the analysis of a module, the module is flagged with No changes.

• read the apriori file present in the openupgrade_scripts project, and add Merged into ..., Renamed to ... and Renamed from ...

The diff result is available here : OCA/OpenUpgrade@6050b84

What do you think ?

[pedrobaeza]

If ---nothing has changed in this module-- is in the analysis of a module, the module is flagged with No changes.

I prefer this to not be done. It's not the same to not have changes in the analysis file than being Nothing to do, and anyways, the only valid wording is this last one, but you shouldn't put it as well, as it should be analyzed.

read the apriori file present in the openupgrade_scripts project, and add Merged into ..., Renamed to ... and Renamed from ...

OK, but note that this should be changed to Done - Renamed... for being tested as the module is installed, so this is still not enough, and we can't put directly the Done word due to OCA/OpenUpgrade#2797.

[legalsylvain]

Hi Pedro. Thanks for your review.
I expressed myself badly, my intention was not to automate the analysis, but to automate the display of useful information, and which are trivial to know.

I refactored to have three columns.

• the second column "Status" should be written by a human. (as you said, it should be analysed).
• the third column contain predictive usefull information and can be prefilled by script.

The result is here. https://github.com/OCA/OpenUpgrade/blob/f6db860bb5c856637a115d8d9ef6367786031291/docsource/modules140-150.rst.
Is it OK for you ?

CC : @StefanRijnhart

[pedrobaeza]

OK, that seems better. Maybe I would rephrase the sentence to No DB layout changes.. Anyways, awesome work, that is assumed in all the communications, but it's not bad to be highlighted explicitly.

[legalsylvain]

Thanks for updating your review !

Maybe I would rephrase the sentence to No DB layout changes

Well, in fact the message is displayed if there are no changes in database AND in XML element. (https://github.com/OCA/OpenUpgrade/blob/15.0/openupgrade_scripts/scripts/account_check_printing/15.0.1.0/upgrade_analysis.txt)

But if you think it's better, I'm with you.

Note : I saw that some info was cumulative : i propose to remove "no changes" / "No DB layout changes" in case of rename / merged (because it is a change by itself).

[StefanRijnhart]

Yes, please remove no changes in case of a rename or merge. Otherwise, very nice!

[legalsylvain]

Yes, please remove no changes in case of a rename or merge. Otherwise, very nice!

Done.

For me, this is ready to merge, except if #2206 (comment) is a blocking point. if so, I'll make the change. Let me know.

CC : @StefanRijnhart, @pedrobaeza

[pedrobaeza]

Yes, please

[legalsylvain]

Yes, please

done.

[StefanRijnhart]

/ocabot merge patch

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-2206-by-StefanRijnhart-bump-patch, awaiting test results.

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2206-by-pedrobaeza-bump-major, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at d1ec921. Thanks a lot for contributing to OCA. ❤️