Skip to content

[FIX] graph: Fix AttributeError - #692

Closed
mvaled wants to merge 1 commit into
OCA:9.0from
mvaled:9.0-safe-eval-import
Closed

[FIX] graph: Fix AttributeError#692
mvaled wants to merge 1 commit into
OCA:9.0from
mvaled:9.0-safe-eval-import

Conversation

@mvaled

@mvaled mvaled commented Dec 6, 2016

Copy link
Copy Markdown

Fixes the following error:

Traceback (most recent call last):
  ...
  File "openupgrade/openerp/modules/graph.py", line 93, in add_modules
    forced_deps = tools.safe_eval.safe_eval(forced_deps)
AttributeError: 'function' object has no attribute 'safe_eval'

@oca-clabot

Copy link
Copy Markdown

Hey @mvaled, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

  • @mvaled (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

Fixes the following error:

```
Traceback (most recent call last):
  ...
  File "openupgrade/openerp/modules/graph.py", line 93, in add_modules
    forced_deps = tools.safe_eval.safe_eval(forced_deps)
AttributeError: 'function' object has no attribute 'safe_eval'
```

Since `eval` is actually the `safe_eval` function we can safely use it.
@mvaled
mvaled force-pushed the 9.0-safe-eval-import branch from 3f89ac8 to de5af59 Compare December 6, 2016 16:32
@mvaled

mvaled commented Dec 6, 2016

Copy link
Copy Markdown
Author

I have sent the OCA CLA by email. Do I need to reopen this PR?

@pedrobaeza

Copy link
Copy Markdown
Member

No, you don't need to do anything on the PR for the CLA, but I'm not sure the issue is real, as the import and the call are correct.

@mvaled

mvaled commented Dec 6, 2016 via email

Copy link
Copy Markdown
Author

@mvaled

mvaled commented Dec 6, 2016

Copy link
Copy Markdown
Author

Hi @pedrobaeza,

Double checking: It seems the change in convert comes from our own changes. So I'm closing this.

In fact the change came from Odoo's commit 12634e1 and the merge in 447179c. Not sure why this is not merged. So this issue came from me, merging Odoo's 9.0 branch prematurely. This change will be needed when merging the Odoo's 9.0 branch into 9.0.

@mvaled mvaled closed this Dec 6, 2016
@mvaled

mvaled commented Dec 6, 2016

Copy link
Copy Markdown
Author

I see that the previous changes around the lines I'm changing were done by @StefanRijnhart in 3426b63. @StefanRijnhart would you think this change will make any sense after we merge again from Odoo's 9.0?

@mvaled mvaled reopened this Dec 6, 2016
@StefanRijnhart

StefanRijnhart commented Dec 9, 2016

Copy link
Copy Markdown
Member

We might not merge again from 9.0 because we don't expect the data model to have changed. But if we do, it should become relevant. To prevent surprises at that time, could you cherry pick the change that lead to the incompatibility and change the call to eval accordingly? Could you make sure that 'eval' is not used as an alias anymore, because it implies unsafe eval? Rather something like s_eval or safe_eval itself.

@mvaled

mvaled commented Dec 9, 2016

Copy link
Copy Markdown
Author

Well, in this module there's single mention of 'eval'.

$ grep eval openerp/modules/graph.py
from openerp.tools.safe_eval import safe_eval as eval
        forced_deps = eval(forced_deps)

I didn't find any import of eval from the graph.py module.

I had to make this merge to include several changes we've done to the core. I'm just beginning to asses the feasibility to upgrade a large installation, so it's early for me to see this is 100% bug-free. But I don't see any downsides so far.

@hbrunn

hbrunn commented Dec 26, 2016

Copy link
Copy Markdown
Member

this change uses the unsaef eval, and #692 (comment) looks like there's some patches upstream used. Quite out of scope here.

@hbrunn hbrunn closed this Dec 26, 2016
@oca-clabot

Copy link
Copy Markdown

Hey @mvaled,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

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.

5 participants