enthought / envisage

Envisage is a Python-based framework for building applications whose functionalities can be extended by adding "plug-ins".
http://docs.enthought.com/envisage/
Other
81 stars 26 forks source link

Fix mistake in menu group specification. #485

Closed prabhuramachandran closed 1 year ago

prabhuramachandran commented 1 year ago

Fixes https://github.com/enthought/mayavi/issues/1182

prabhuramachandran commented 1 year ago

@rahulporuri -- thanks, not sure what the pip errors are here but requesting someone to please merge this when possible.

mdickinson commented 1 year ago

We do need to get to the bottom of the CI issues before we merge, even if that's just verifying that the same issue exists on the main branch, so that it's independent of this PR.

Unfortunately I won't have bandwidth for that in the near future.

mdickinson commented 1 year ago

I ran the test-with-pypi workflow on main; indeed it's failing in the same way: https://github.com/enthought/envisage/actions/runs/3264826259

mdickinson commented 1 year ago

The issue appears to be coming from Pyface: https://github.com/enthought/pyface/issues/1163

mdickinson commented 1 year ago

Changes look good. But this is a bugfix, so it should really have a regression test. @prabhuramachandran: is there something that could be stolen from Mayavi that would act as a suitable regression test?

prabhuramachandran commented 1 year ago

I have added the smallest possible test, a simple import catches the error. I hope this is good enough?

prabhuramachandran commented 1 year ago

@mdickinson -- I added probably the most trivial of tests. It fails without the changes and passes with them.

prabhuramachandran commented 1 year ago

Looks like I may have messed up something in the commit, I am not sure but the test errors seem unrelated here.

prabhuramachandran commented 1 year ago

@mdickinson -- is there anything else needed for this?

mdickinson commented 1 year ago

is there anything else needed for this?

Just the flake8 fix, I think (see https://github.com/enthought/envisage/pull/485/commits/754bd86ea9cfbb5a896c633394dfe9e145d4e11d). I'll merge when/if the checks pass.

mdickinson commented 1 year ago

Merged. Thanks for the fix.