Skip to content
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

[ADD] Migration scripts for the project addon #80

Merged
merged 1 commit into from Nov 3, 2014

Conversation

bwrsandman
Copy link

Revert and re-open #65.

@bwrsandman
Copy link
Author

After comments from @red15, I decided to revert the merge.

@StefanRijnhart
Copy link
Member

Resubmit

@@ -367,7 +372,7 @@ def logged_query(cr, query, args=None):
if args is None:
args = []
cr.execute(query, args)
logger.debug('Running %s', query % tuple(args))
logger.debug('Running %s', query % args)
Copy link
Author

Choose a reason for hiding this comment

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

This will cause a regression on a bug when args was passed as a list.
cr.execute was ok with lists IIRC
EDIT, just look at L373:

args = []

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is not very pretty but it is trivial of course:

'dfsa' % []
'dfsa'

On my initial review I checked that all occurrences I could find in openupgrade-addons/7.0 pass a tuple, not a list. Do you have a concrete example where a list is passed that would trigger a regression? In fact, does that not break cr.execute which I believe does not take a list of arguments very well instead of a tuple?

Copy link
Author

Choose a reason for hiding this comment

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

>>> "%s, %s" % ["1", "2"]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not enough arguments for format string

I remember running into this bug in 7.0 migration, I was calling logged query with a list.
I am resposible for the tuple being there in the first place.
If we support args as a dict, why not proof it for lists as well.
I don't want this back and forth of args and tuple(args) to go on forever ;)

And the concrete example is right there, two lines above:

     if args is None:
         args = []

It seems really senseless to proof it with None to the cr.execute and then have it fail on the next line.
These four lines should be compatible with eachother.

Edit: I realise that [] is an edge case which will not cause a fail, but I think my point still stands. If I look at the code, it tells me that args is expected to be a list. I will therefore use a args as a list and run into this bug.

Copy link
Member

Choose a reason for hiding this comment

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

I now see that you actually can pass a list to cursor.execute(), so we should settle for
args = tuple(args) if typeof(args) == list else args then

-- EDIT-- Done.

@bwrsandman
Copy link
Author

@StefanRijnhart > is for quotes, if you're doing code surround it with backticks () for inline code`
or use triple backticks () on the line above and bellow a code block, additionally if your leading backticks include the language (python), it will highlight it for you.

>>> 'dfsa' % []
>>> 'dfsa'

That information and more is available if you click on "Parsed as Markdown" in the write dialog.

@bwrsandman
Copy link
Author

👍

if not model:
logger.exception("map_values is called with no table and no model")
table = model._table
for old, new in mapping:
Copy link
Member

Choose a reason for hiding this comment

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

Please add a check for avoiding using the same column as source and target.

@StefanRijnhart
Copy link
Member

@pedrobaeza Thanks, I backported Sylvain's change to the other branch here.

@StefanRijnhart
Copy link
Member

I see there are conflicts

@StefanRijnhart
Copy link
Member

Rebased and conflicts resolved

@hbrunn
Copy link
Member

hbrunn commented Nov 3, 2014

👍

legalsylvain added a commit that referenced this pull request Nov 3, 2014
@legalsylvain legalsylvain merged commit dd28190 into OCA:8.0 Nov 3, 2014
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.

None yet

5 participants