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

Fix mistake in menu group specification. #485

Merged
merged 6 commits into from
Nov 5, 2022

Conversation

prabhuramachandran
Copy link
Member

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

This looks similar to other fixes we made recently.

@prabhuramachandran
Copy link
Member Author

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

@mdickinson
Copy link
Member

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
Copy link
Member

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
Copy link
Member

The issue appears to be coming from Pyface: enthought/pyface#1163

@mdickinson
Copy link
Member

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
Copy link
Member Author

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

@prabhuramachandran
Copy link
Member Author

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

@prabhuramachandran
Copy link
Member Author

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

@prabhuramachandran
Copy link
Member Author

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

@mdickinson
Copy link
Member

is there anything else needed for this?

Just the flake8 fix, I think (see 754bd86). I'll merge when/if the checks pass.

@mdickinson mdickinson merged commit f23ea38 into enthought:main Nov 5, 2022
@mdickinson
Copy link
Member

Merged. Thanks for the fix.

mdickinson pushed a commit that referenced this pull request Feb 6, 2023
Fixes enthought/mayavi#1182

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
(cherry picked from commit f23ea38)
mdickinson pushed a commit that referenced this pull request Feb 7, 2023
Fixes enthought/mayavi#1182

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
(cherry picked from commit f23ea38)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants