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

Fixing duplicated definition of magic aliases after reset #11806

Merged
merged 3 commits into from Jul 23, 2019

Conversation

@juanis2112
Copy link
Contributor

@juanis2112 juanis2112 commented Jun 28, 2019

Fixes #11646 by avoiding the double definition of the magic aliases.

@Carreau
Copy link
Member

@Carreau Carreau commented Jun 29, 2019

Hi @juanis2112 !

Thanks for your PR ! I'll try to review it now.

Loading

try:
name = self.magics_manager.magics['line'][cmd]
except KeyError:
self.alias_manager.soft_define_alias(cmd, cmd)
Copy link
Member

@Carreau Carreau Jun 29, 2019

Choose a reason for hiding this comment

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

Do you have a reasoning to use try:...Except KeyError instead of checking:

if cmd not in self.magics_manager.magics['line']:
    self.alias_manager.soft_define_alias(cmd, cmd)

I would tend to have a preference for the if ... version , as try/except could swallow bugs in soft_define_alias, but I could easily be convinced.

Loading

Copy link
Member

@ccordoba12 ccordoba12 Jul 1, 2019

Choose a reason for hiding this comment

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

I agree with @Carreau here: I think it's better to use the ifversion and not the try/except one here.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jun 29, 2019

That is great ! I'll see if I can add a test to make sure there is no later regressions ! Unless you want to add a test; I'm happy to guide you in writing one if you desire.

Loading

@ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jul 1, 2019

Thanks a lot for this @juanis2112!

@Carreau, @juanis2112 is working with me in Spyder and I asked her to fix #11646 because it's really annoying for Spyder users. Could you guide her to write a regression test for this? Thanks!

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jul 1, 2019

Could you guide her to write a regression test for this? Thanks!

Of course I will.

Side question; it seem GH allow us to give only triage permission to repositories (close issues and manage labels); would that be of interest to you @ccordoba12 ?

Loading

@juanis2112
Copy link
Contributor Author

@juanis2112 juanis2112 commented Jul 3, 2019

@Carreau I'll be happy to help with the test, with a bit of guidance as @ccordoba12 suggested.

Loading

@ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jul 4, 2019

would that be of interest to you ?

Sorry, but no. I do the same job for almost all repos in the Spyder organization, so I really don't have time to help you with that.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jul 7, 2019

Sorry, but no. I do the same job for almost all repos in the Spyder organization, so I really don't have time to help you with that.

It was morre incase there was spyder specific issues here so that you could properly tag them; and more giving more rights that requesting more from you.

I'll be happy to help with the test

Thanks, I'll try to lay out what I believe we should try to do.

  1. the issue appear only when magics are already registered with the same name, which mean i out test we will wan to register a fake magic.

  2. We also likely want to use AssertNotPrints to make sure it does not prints said mesage.

  3. We can likely put a test in IPython/core/tests/test_magic.py.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jul 7, 2019

Ok, so I looked into it a bit more carefully and it is slightly more complicated to write test, so here is what I came up with:

modified: IPython/core/tests/test_magic.py
@ test_magic.py:374 @ def test_reset_in_length():
     _ip.run_cell("reset -f in")
     nt.assert_equal(len(_ip.user_ns['In']), _ip.displayhook.prompt_count+1)

+class TestResertFErrors(TestCase):

+    def test_reset_redefine(self):

+        @magics_class
+        class KernelMagics(Magics):
+              @line_magic
+              def less(self, shell): pass

+        _ip.register_magics(KernelMagics)

+        with self.assertLogs() as cm:
+            # hack, we want to just capture logs, but assertLogs fails if not
+            # logs get produce.
+            # so log one things we ignore.
+            import logging as log_mod
+            log = log_mod.getLogger()
+            log.info('Nothing')
+            # end hack.
+            _ip.run_cell("reset -f")

+        assert len(cm.output) == 1
+        for out in cm.output:
+            assert "Invalid alias" not in out

 def test_tb_syntaxerror():
     """test %tb after a SyntaxError"""
     ip = get_ipython()

You can use it as-is, or try to come up with a better way, but it's tricky enough that I can't request you to write an annoying test case like that.

If you just want to change your code from try/except to if cmd not in: self.magics_manager.magics['line']:, I can take care of adding the test, and we can get that in for 7.7.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Jul 23, 2019

@juanis2112 You've done some great contribution, as I wan to release 7.7. soon; I've pushed the test on your branch; and also switched from Try-except to if ... in. I'm going to merge to make sure this contribution get into the next release.

Thanks Again and looking forward to your next contribution.

Loading

@Carreau Carreau merged commit d56fd37 into ipython:master Jul 23, 2019
2 checks passed
Loading
@Carreau Carreau added this to the 7.7 milestone Jul 23, 2019
@ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Aug 8, 2019

@Carreau, thanks a lot for helping @juanis2112 to finish this PR!! I instructed her to work on other Spyder issues in the meantime and we totally forgot about this!

Loading

@ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Aug 8, 2019

It was morre incase there was spyder specific issues here so that you could properly tag them; and more giving more rights that requesting more from you.

In that case I accept your offer. I didn't know about the new Github functionality you mentioned above, and I think it'd be really useful in this case.

Loading

@Carreau
Copy link
Member

@Carreau Carreau commented Aug 8, 2019

I instructed her to work on other Spyder issues in the meantime and we totally forgot about this!

No problem, the hard part of the work was down, and it was only annoying details to finish.

In that case I accept your offer. I didn't know about the new Github functionality you mentioned above, and I think it'd be really useful in this case.

I'm not sure the feature is available on all orgs yet, hence why you might not have seen it.Ok, I'll invite you to the right team for that; let me know if you know any body else that would like to be added to triage.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants