Skip to content

Conversation

@yajo
Copy link
Member

@yajo yajo commented Aug 4, 2020

As a remainder from 8d1cb34 (possibly trying to fix the wrong problem), this test was being ran as post install. This broke some integration environments without need.

  • Removed post install mode.
  • Improved to use SavepointCase.

@Tecnativa TT23969

As a remainder from 8d1cb34 (possibly trying to fix the wrong problem), this test was being ran as post install. This broke some integration environments without need.

- Removed post install mode.
- Improved to use `SavepointCase`.

@Tecnativa TT23969
@pedrobaeza pedrobaeza added this to the 10.0 milestone Aug 4, 2020
@pedrobaeza
Copy link
Member

For reducing the diff, please don't rename all self occurrences by cls, just add a first line like:

self = cls

I wonder if this will break other things, and we only having one DB in 10.0, not sure if it's worth.

@yajo
Copy link
Member Author

yajo commented Aug 4, 2020

I prefer to have more diff but let code be more explicit.

@pedrobaeza
Copy link
Member

The code is the same explicit... Anyway, as we are moving the customer to upper versions, we can let this as PR and never merge.

@pedrobaeza
Copy link
Member

Look your inconsistency also: here you want to be "more explicit", and in OCA/maintainer-tools#466 you want to be less explicit? Choose one.

@yajo
Copy link
Member Author

yajo commented Aug 4, 2020

OCA/maintainer-tools#466 is unrelated. Also, the point that Tecnativa has more or less customers is not relevant for OCA when judging this PR. Personally, when a PR is not interesting to me because it doesn't affect our customers, I just unsubscribe and let other members review; you can do so if this is not interesting to you 🤷‍♂️ . In any case, please don't mix subjects.

Could we please focus on the PR? Is there any real blocking point, or can we merge? 🙄

@pedrobaeza
Copy link
Member

It was related about consistency in the principles used: you should be in general explicit or implicit, and Python already told us about being explicit, so it would be OK in this PR, but not in the other (that you remove the explicit part). However, this one can be considered an exception, as it's an old branch with a very specific case that can be solved with few code lines, so this is why I asked for not doing such diff.

Anyway, outside of the diff, I still think that this can produce side effects, so it's better to keep it as a PR as we only needed for one customer CI that is in process of being migrated, and we can add this PR to it.

@pedrobaeza
Copy link
Member

Let's close it for having a clean house in our tracker, but not removing the branch for their usage in the CI.

@pedrobaeza pedrobaeza closed this Aug 13, 2020
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