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

Add suggested features #1

Merged
merged 12 commits into from
Apr 4, 2022
Merged

Add suggested features #1

merged 12 commits into from
Apr 4, 2022

Conversation

winniederidder
Copy link
Collaborator

This PR adds the features discussed here.
The code is backed by a file using set_source_code, which is called in pre_run.
Overrides for builtins are set in pre_run.

@winniederidder
Copy link
Collaborator Author

winniederidder commented Apr 3, 2022

@alexmojaki Am I reading the output correctly in that you expect the faulty line to not be present in the expected output?
I updated the tests to expect the faulty line and indication of failure.
The KeyboardInterrupt-handling was also changed.

@coveralls
Copy link

coveralls commented Apr 3, 2022

Pull Request Test Coverage Report for Build 2089795818

  • 29 of 29 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2012234990: 0.0%
Covered Lines: 175
Relevant Lines: 175

💛 - Coveralls

@@ -35,6 +35,22 @@ def __init__(
def set_callback(self, callback):
self._callback = callback

def set_source_code(self, source_code, filename):
Copy link
Owner

Choose a reason for hiding this comment

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

This signature is weird. Internally, only one argument is really being used at a time. Is it because of something you want to do externally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it is linked to keeping the source code as an attribute. However, to work properly, the source code needs a filename attached to it to have a proper meaning. Both arguments are also used, so I'm not sure what you mean by one at a time?

Copy link
Owner

Choose a reason for hiding this comment

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

What I meant was that either it was only really setting filename (with source_code="") or only really setting source_code (since filename=self.filename doesn't do much).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The source code could be different from "" in the constructor. However, setting the file separately might also make more sense. If you are changing between files in your editor, you probably don't want to run it immediately but still inform the runner of changes. I'll split it into two methods.

tests/test_runner.py Outdated Show resolved Hide resolved
python_runner/runner.py Outdated Show resolved Hide resolved
@alexmojaki
Copy link
Owner

I think your idea of making source_code an attribute is a good one.

python_runner/runner.py Outdated Show resolved Hide resolved
@alexmojaki
Copy link
Owner

Thanks!

@alexmojaki alexmojaki merged commit c2f4955 into master Apr 4, 2022
@winniederidder
Copy link
Collaborator Author

@alexmojaki Will this then be released on PyPi so I can refactor my own Python code in Papyros for these changes?

@alexmojaki
Copy link
Owner

Done.

@winniederidder winniederidder deleted the papyros-features branch April 12, 2022 11:36
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.

None yet

3 participants