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.

Sign up for GitHub

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

Jump to bottom

Bugfix for #28 #41

Merged
merged 1 commit into from
Aug 23, 2017
Merged

Bugfix for #28 #41

merged 1 commit into from
Aug 23, 2017

Conversation

patrick-nicholson
Copy link
Contributor

Quick patch to enable %%scala magic when running under ipykernel.

@patrick-nicholson
Copy link
Contributor Author

#28

@codecov
Copy link

codecov bot commented Aug 23, 2017 •
edited
Loading

Codecov Report

Merging #41 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   82.68%   82.73%   +0.04%     
==========================================
  Files           6        6              
  Lines         387      388       +1     
==========================================
+ Hits          320      321       +1     
  Misses         67       67

parente
parente reviewed Aug 23, 2017
View reviewed changes
spylon_kernel/scala_magic.py
@@ -29,7 +29,7 @@ class ScalaMagic(Magic):
def __init__(self, kernel):
super(ScalaMagic, self).__init__(kernel)
self.retval = None
self._interp = None
self._interp = scala_intp
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. Is this really needed? _get_scala_interpreter initializes this variable by calling get_scala_interpreter() instead of reaching in and grabbing the global directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not necessary. It's really just to avoid the messaging, etc. around using the magic for the first time if an interpreter already exists.

@patrick-nicholson
Copy link
Contributor Author

@parente This PR is just a band-aid to get %%scala working. I'd like to really fix it by refactoring the package a bit, but we should talk about the refactoring first. If this doesn't do enough in the interim, we can close this.

@parente parente merged commit d880daf into vericast:master Aug 23, 2017
@parente
Copy link
Contributor

parente commented Aug 23, 2017

Merged. As for refactoring, yes. The cross-calling happening between the interpreter class and the magic makes the code hard to follow and maintain for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parente parente parente left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@patrick-nicholson @parente