Skip to content

[ADD] base, base_setup, web, web_editor, web_unsplash#2205

Merged
StefanRijnhart merged 3 commits intoOCA:13.0from
kos94ok-3D:13.0-base
Feb 28, 2020
Merged

[ADD] base, base_setup, web, web_editor, web_unsplash#2205
StefanRijnhart merged 3 commits intoOCA:13.0from
kos94ok-3D:13.0-base

Conversation

@kos94ok-3D
Copy link

My first steps in the migration

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@kos94ok-3D kos94ok-3D force-pushed the 13.0-base branch 8 times, most recently from 3aeb29c to 2fcb66b Compare February 25, 2020 11:50
@kos94ok-3D
Copy link
Author

Hi, @MiquelRForgeFlow.
Can you help me? Is it a problem with Travis or I missing something?

@MiquelRForgeFlow
Copy link
Contributor

I'm busy with other things, sorry but it's not my priority right now

@kos94ok-3D
Copy link
Author

Hi, @StefanRijnhart .
Have you any time in order to help me? Is it a problem with Travis or I missing something?

@kos94ok-3D kos94ok-3D force-pushed the 13.0-base branch 4 times, most recently from e20a7db to b27d188 Compare February 26, 2020 13:16
@kos94ok-3D kos94ok-3D changed the title [ADD] Base [ADD] base, base_setup, web, web_editor, web_unsplash Feb 26, 2020
@kos94ok-3D kos94ok-3D marked this pull request as ready for review February 26, 2020 13:45
@kos94ok-3D
Copy link
Author

HI, @pedrobaeza.
Maybe you have any idea about an error in Travis.

@pedrobaeza
Copy link
Member

Sorry, I'm not either on this.

@StefanRijnhart
Copy link
Member

Very nice work, thank you! Travis failure might be related to an issue with the port of the OpenUpgrade framework to Odoo 13, c.q. running Openupgrade specific tests (of which there are none yet). Maybe you can reproduce the test error by running locally on an odoo 12 demodatabase with the OPENUPGRADE_TESTS=1 environment variable set?

@kos94ok-3D
Copy link
Author

kos94ok-3D commented Feb 27, 2020

Very nice work, thank you! Travis failure might be related to an issue with the port of the OpenUpgrade framework to Odoo 13, c.q. running Openupgrade specific tests (of which there are none yet). Maybe you can reproduce the test error by running locally on an odoo 12 demodatabase with the OPENUPGRADE_TESTS=1 environment variable set?

Thank you.
I found two problems:

  1. importlib.import_module can not correctly work with names which contains dots
  2. tools.config['test_tags'] return None if not set so we get Traceback

What to do? Someone use migration tests? (I fixed it in #2207)

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Great work, thanks! I had been doing a bit of work on this module and I have a small number of minor additions that I will propose when this is merged.

@kos94ok-3D
Copy link
Author

kos94ok-3D commented Feb 28, 2020

Great work, thanks! I had been doing a bit of work on this module and I have a small number of minor additions that I will propose when this is merged.

Thank you for review.
OK

@StefanRijnhart StefanRijnhart merged commit 1c0027a into OCA:13.0 Feb 28, 2020
@kos94ok-3D kos94ok-3D deleted the 13.0-base branch February 28, 2020 15:21
@MiquelRForgeFlow
Copy link
Contributor

@StefanRijnhart Is there such a hurry as to not wait for other opinions before merge?

One thing is that I said I can't "help" because I'm busy with other things and another thing is that I can't say something before it gets merged. It's not a trivial PR and more work needs to be done in base.

@pedrobaeza
Copy link
Member

Yeah, I agree, because in documentation it mentions now that base module is Done, and possible issues that arises derived from it must be now attended. Better to remove that tag or put something like "tentative".

@StefanRijnhart
Copy link
Member

Well, again I think projects like openupgrade benefit from optimistic merging, in which incomplete and suboptimal code can be merged if it constitutes an improvement and when tests are green.

@pedrobaeza
Copy link
Member

As said, the problem is to qualify this "experimental" code (although of course it's a great achievement from a new contributor) as "Done" in the documentation.

@StefanRijnhart
Copy link
Member

Follow up here: #2210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants