Skip to content

Add display impl, rm outputManage, print and console.log default to browser console#749

Merged
marimeireles merged 35 commits into
pyscript:mainfrom
marimeireles:marimeireles/output
Oct 17, 2022
Merged

Add display impl, rm outputManage, print and console.log default to browser console#749
marimeireles merged 35 commits into
pyscript:mainfrom
marimeireles:marimeireles/output

Conversation

@marimeireles

@marimeireles marimeireles commented Sep 5, 2022

Copy link
Copy Markdown
Contributor

This is a WIP, still need to work in tests, but @antocuni wants to see the code, =)

Closes #635 #634 #230 #472 #280 #230 #712 #758 #76 #103

TODO: Create test cases for: #230 #472

@marimeireles marimeireles marked this pull request as draft September 5, 2022 15:36
Comment thread pyscriptjs/src/python/pyscript.py Outdated
)


current_py_script_tag = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

UPPERCASE ;)
Or, even better: instead of using a global, you can store the value as an attribue of the function object itself:

def display():
    ...
display._default_target_id = None

def set_default_display_target_id(id):
    display._default_target_id = id

@marimeireles marimeireles Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops
true about the uppercase
alright 👍 I did it like this cause you said global was the best solution? but you actually just mean we shouldn't use an object?
Also, I don't understand why it's a problem to use an object. I think it's possible to just pass it on the base.ts, on the same place where I call the set_current_display_target function. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did it like this cause you said global was the best solution? but you actually just mean we shouldn't use an object?

yes exactly. What I meant was "using global state is simpler than injecting a per-script object".
That said, you can set global state in two ways:

  1. with a "real" global variable, as you did
  2. as an attribute of the global display function

The difference is shallow, but usually in Python (2) is preferred because it makes it clear that the global is specific to this function

Also, I don't understand why it's a problem to use an object. I think it's possible to just pass it on the base.ts, on the same place where I call the set_current_display_target function. What do you think?

feel free to try and tell me how it goes :)

@marimeireles marimeireles force-pushed the marimeireles/output branch 2 times, most recently from 4647165 to 31ce5bc Compare September 5, 2022 17:08
@marimeireles

Copy link
Copy Markdown
Contributor Author

I'd like to propose a small refactor for this PR.
The way we output errors on the REPL is not ideal and the red banner by the footer of the page is not pretty, IMO.
I'd like to implement by default that all error messages show up underneath the REPL tag that errored with an UI that copies the error logs on the browser console. (Light red background with red colored letters in a red lined box)
What are folks opinions on this? @fpliger @antocuni

@antocuni

antocuni commented Sep 7, 2022

Copy link
Copy Markdown
Contributor

I'd like to propose a small refactor for this PR. The way we output errors on the REPL is not ideal and the red banner by the footer of the page is not pretty, IMO. I'd like to implement by default that all error messages show up underneath the REPL tag that errored with an UI that copies the error logs on the browser console. (Light red background with red colored letters in a red lined box) What are folks opinions on this? @fpliger @antocuni

I don't have any strong opinion.
+0 to improve the style, although it looks OT with this PR.

Comment thread pyscriptjs/src/python/pyscript.py Outdated
Comment on lines +127 to +139
CURRENT_PY_SCRIPT_TAG = None


def set_current_display_target(element):
global CURRENT_PY_SCRIPT_TAG
CURRENT_PY_SCRIPT_TAG = element


def display(value, parent=None, append=True):
if parent is None:
global CURRENT_PY_SCRIPT_TAG
parent = CURRENT_PY_SCRIPT_TAG
Element(parent).write(value, append)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@antocuni I forgot how to declare a global in a class... could you please show me again? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"a global in a class" -- I don't know what it means :). But maybe you are referring to this pattern:

def set_current_display_target(element):
    get_current_display_target._obj = element

def get_current_display_target():
    return get_current_display_target._obj

get_current_display_target._obj = None

I.e., instead of storing it in a global, you store it as an attribute of get_current_display_target.

@marimeireles marimeireles marked this pull request as ready for review September 14, 2022 17:59
@marimeireles

Copy link
Copy Markdown
Contributor Author

tests are a WIP
I don't understand why the make fmt didn't catch this one line that the flake8 caught in the CI. Is this something we have it on the CI and not on the Makefile maybe? Or am I using the wrong thing.

Comment thread pyscriptjs/src/styles/pyscript_base.css Outdated
Comment on lines +5 to +28
.hidden {
visibility: hidden;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need this, TODO for me, test it tomorrow.

@marimeireles

Copy link
Copy Markdown
Contributor Author

Okay... Tests run 100% locally to me but not on the CI 😔

console.info
    [pyscript/pyodide] Runtime config: {
      name: 'pyodide-default',
      lang: 'python',
      src: 'https://cdn.jsdelivr.net/pyodide/v0.21.2/full/pyodide.js'
    }

      at Object.out_fn [as info] (src/logger.ts:50:13)

  console.info
    [pyscript/pyodide] Loading pyodide

      at Object.out_fn [as info] (src/logger.ts:50:13)

make: *** [Makefile:93: test-ts] Error 1

Seems related to logger stuff from Antonio? But I rebased so ???
I'll check it tomorrow morning cause it's getting late for me.

@fpliger fpliger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

think this PR is very close to ready to approve. Only needs to fix tests.. or am I missing something @marimeireles ?

Comment thread pyscriptjs/src/python/pyscript.py Outdated
Comment on lines +126 to +150
CURRENT_PY_SCRIPT_TAG = None


def set_current_display_target(element):
global CURRENT_PY_SCRIPT_TAG
CURRENT_PY_SCRIPT_TAG = element


def display(value, parent=None, append=True):
if parent is None:
global CURRENT_PY_SCRIPT_TAG
parent = CURRENT_PY_SCRIPT_TAG
Element(parent).write(value, append)


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's ok to start from here but think there are ways we can improve and isolate display "current target" to the scope of execution so that we don't end up with race conditions of different scopes overwriting on each other.

@philippjfr suggestion of ContextVar in #769 is really interesting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah!
I need @antocuni's input here on how to make it prettier.
I don't wanna use this kind of global 🙈

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But I intend to give it a try on the ContextVar stuff for sure =)
I'm updating the issue ^

@fpliger fpliger Sep 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! @antocuni , we have another opportunity to kindly disagree and trade beers! 😝

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's ok to start from here but think there are ways we can improve and isolate display "current target" to the scope of execution so that we don't end up with race conditions of different scopes overwriting on each other.

@fpliger
sorry, reading it only now. I think that what you are proposing is impossible.
The semantics of Python scopes clashes with the desired semantics of display() in which the implicit target depends on the caller.
The only way to implement it (see below*) is to use a global variable. Or at least, that's the conclusion which I came up with after thinking a lot about the issue.
ContextVar helps with the async case but not in general, because it's essentially a "coroutine-aware global variable" -- so long as you stay in the same coroutine you have the very same problems as global variables.

I think that we will regret this choice: it's not really compatible with python semantics and will cause subtle bugs and problems in the future, but I'm not willing to fight for it :).


"the only way to implement it": well, the only reasonable way. The unreasonable way would be to modify the AST and/or the bytecode to detect the usage of display() and do some deep magic.
But if we go down this route, I'll resign immediately 😂

@JeffersGlass JeffersGlass Oct 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@antocuni Apologies for butting in on this, and the following clarifying questions shouldn't delay this (quite good!) PR:

The semantics of Python scopes clashes with the desired semantics of display() in which the implicit target depends on the caller... ContextVar helps with the async case but not in general, because it's essentially a "coroutine-aware global variable" -- so long as you stay in the same coroutine you have the very same problems as global variables.

I did want to clarify - what are the situations where you're in the same coroutine but you have different callers (different implicit targets)? Doesn't a single coroutine inPyScript have a single caller and therefore a well-defined implicit target that can be tracked via a ContextVar?

What I think is probably best is for me to hush up for awhile, and have another spin at this once this PR is merged 😅. Probably I just have an outdated understanding of the desired semantics here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did want to clarify - what are the situations where you're in the same coroutine but you have different callers (different implicit targets)?

that's my point, we don't know 😅.
Using global state to pass around function arguments is a bad API design: it makes functions not re-entrant and in general leads to subtle bugs and behavior. Implicit global state is even worse.
My point is that this approach is inherently fragile and it's "wrong by default": if we can think of all possible corner cases we can put workarounds for them, but as soon as we forget about one we will see bugs.

A random examples of things which might happen:
currently we do set_display_target(X); ...; set_display_target(None), but if for any reason we end up calling this code recursively, the inner invocation would set the target to None and then the outer one would see it changing under its feet. AFAIK this is not something which can happen now but might happen in the future (maybe without even noticing). And also it would be easy to fix, you just need a stack of current targets instead of just a variable.
This might happen even sooner than we think: for example, what happens if you add a <py-script> tag to the DOM from python code? I don't know what is the desired semantics, but if by chance we decide that the python code needs to be executed immediately, then you are in the exact situation which I described above.

Another example is event handlers: when thinking about the issue I realized that event handlers were a problem, but god only knows how many other problematic situations there are that we couldn't think of.

Comment thread pyscriptjs/src/components/base.ts Outdated
Comment on lines +124 to +128
<string>await runtime.run(`set_current_display_target(element="${this.id}")`);
<string>await runtime.run(source);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@antocuni this is where we fetch the id.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do you need the <string> prefix?
(Genuine question, there might be a good typescript reason which I don't know)

@tedpatrick tedpatrick Oct 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The <string> casts the return type to a string in TS.

Pyodide returns any from calls to runPython and runPythonAsync but the result does have a well-defined typed result being a union of boolean | string | number | Proxy... oh my!

type PyodideResult = boolean| string | number | Proxy | undefined

const result0:Promise<PyodideResult> = await pyodide.runPython(code );
const result0:Promise<PyodideResult> = await pyodide.runPythonAsync(code );

I see no issues with this syntax as the TS Strict work should improve this in time.

@antocuni antocuni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good job 💪
Although it definitely misses tests 🍻

out_element.appendChild(script_element)
else:
elif hasattr(out_element, "innerHTML"):
out_element.innerHTML = html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this look like a code smell. Why do we need to check whether out_element has "innerHTML"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we need this anymore. I think it's the result of some previous behavior that I can't reproduce anymore.
But, why is it a code smell?
It's not better to be more specific about what we want?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I think that the code is outdate, but answering just for the sake of conversation)

I think it's the result of some previous behavior that I can't reproduce anymore.

if you can kill a line of code without causing any fail to test, it either means that the line of code is useless (and so it should be removed) or tests are missing (and so they should be added).

But, why is it a code smell?

out_element is supposed to be an instance of Element and as such it always have the innerHTML property. If it doesn't it means that we are passing something of the wrong type, but then this is the wrong place where to fix the problem: it should be fixed earlier and avoiding passing an out_element which is not actually an element.



pyscript = PyScript()
output_manager = OutputManager()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hoorray for killing a lot of code 🎉

Comment thread pyscriptjs/src/components/base.ts Outdated
Comment on lines +124 to +128
<string>await runtime.run(`set_current_display_target(element="${this.id}")`);
<string>await runtime.run(source);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do you need the <string> prefix?
(Genuine question, there might be a good typescript reason which I don't know)

Comment thread pyscriptjs/src/python/pyscript.py

@antocuni antocuni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very good job @marimeireles 🎉.
I read and reviewed your commits one by one, in true TDD spirit and you did a very good job at following it 😍.
I left many comments, but don't be alarmed: most of them are just style/suggestions/nitpicks that can be ignored, but I hope that you can find them useful.
A couple of comments are real issues that needs to be addressed though. But as I said, in general I'm very happy with the overall shape of this PR

Comment thread pyscriptjs/src/components/base.ts Outdated
<string>await runtime.run(`set_current_display_target(element="${this.id}")`);
<string>await runtime.run(`set_current_display_target(target_id="${this.id}")`);
<string>await runtime.run(source);
<string>await runtime.run(`set_current_display_target(target_id=None)`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[fix] if runtime.run(source) raises an exception, the current_display_target is never set back to None.
There should be a try/finally (and a test!)

Note: I'm reviewing commits one by one, so it could be that this is already fixed by next commits. Feel free to ignore in that case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I completely forgot about it.
This is what you mean:

            try {
                <string>await runtime.run(`set_current_display_target(target_id="${this.id}")`);
                <string>await runtime.run(source);
            } finally {
                <string>await runtime.run(`set_current_display_target(target_id=None)`);
            }

correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes. And ideally, with a test ;)
The test is a bit hard to write, but I I guess that with the current code the following test would fail:

  1. execute a <py-script> which raises an exception
  2. call display() from an event handler: in theory it should fail (because you cannot call display() without explicit target form event handlers), but with the current code it will pass because the current_target was set in (1) and never reset to None.

So the test should check that (2) fails.

Comment thread pyscriptjs/src/python/pyscript.py Outdated
# XXX fixme
assert "1 1" == inner_text
text = self.page.text_content("body")
assert "hello" in text

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm confused. If I understand correctly, in this test we want to check that hello in displayed in a very specifiy place (second-pyscript-tag), so you should be more precised and get the text_content() of it, instead of the text_content of the whole body.

Comment thread pyscriptjs/tests/integration/test_02_output.py
def display_hello():
# this fails because we don't have any implicit target
# from event handlers
display('hello', target='my-button')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above, the comment is wrong

def test_image_display(self):
self.pyscript_run(
"""
<py-env>- matplotlib</py-env>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you will notice it as soon as you rebase, but <py-env> is gone. You should use <py-config>

console_text = self.console.all.lines
assert 'print from python' in console_text
assert 'print from js' in console_text
assert 'error from js' in console_text

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nitpick] note that with self.console you can do much more precise asserts if you want. E.g. you could say:

assert self.console.log.lines[-2] == 'print from python'
assert self.console.log.lines[-1] == 'print from js'
assert self.console.error.lines == ['error from js']

)
console_text = self.console.all.lines
assert console_text.index('1print') == (console_text.index('2print') - 1)
assert console_text.index('1console') == (console_text.index('2console') - 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[style] the name of the variable is wrong: I would expect console_text to be a string (like console.text), but here it's a list.

Also, you can be much more concrete in your test: here you are basically introducing some semi-complex logic to check that the code when to consecutive but different lines, but you can probably do something much simpler:

lines = self.console.log.lines
assert lines[-4] == '1print'
assert lines[-3] == '2print'
assert lines[-2] == '1console'
assert lines[-1] == '2console'

Generally speaking, it is better to have tests which contains very straightforward logic: they are easier to read, there is less probability that the test itself is buggy, and it's much easier to understand what's wrong in case of failure.

But again this is a nitpick and the test is already very good 💪 , so feel free to ignore this comment if you want

console_text = self.console.all.lines
assert 'print from python' in console_text
assert 'print from js' in console_text
assert 'error from js' in console_text

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah cool, I think this fixes my previous comment. Good job! 💪

import asyncio
for i in range(2):
display('A')
await asyncio.sleep(0.1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why 0.1? The standard python way to say "please switch to another coroutine" is await asyncio.sleep(0)

@marimeireles

Copy link
Copy Markdown
Contributor Author

The REPL test that's failing really doesn't make sense... I think they're just haunted. It runs on my machine.

@marimeireles

marimeireles commented Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Ok, so this beauty is finally green, but I had to add a weird fix.
I need people who understands py-config here... So maybe @madhur-tandon, @antocuni?
I had the following problem:

This test would work fine on the CI:

    def test_runtime_config(self, tar_location):
        unzip(
            location=tar_location,
            extract_to=self.tmpdir,
        )

        self.pyscript_run(
            """
            <py-config type="json">
                {
                    "runtimes": [{
                        "src": "/pyodide/pyodide.js",
                        "name": "pyodide-0.20.0",
                        "lang": "python"
                    }]
                }
            </py-config>

            <py-script>
                import sys, js
                pyodide_version = sys.modules["pyodide"].__version__
                js.console.log("version", pyodide_version)
                display(pyodide_version)
            </py-script>
        """,
        )

        assert self.console.log.lines == [self.PY_COMPLETE, "version 0.20.0"]
        version = self.page.locator("py-script").inner_text()
        assert version == "0.20.0"
        

This one wouldn't:

    def test_runtime_config(self, tar_location):
        unzip(
            location=tar_location,
            extract_to=self.tmpdir,
        )

        self.pyscript_run(
            snippet="""
            <py-script>
                import sys, js
                pyodide_version = sys.modules["pyodide"].__version__
                js.console.log("version", pyodide_version)
                display(pyodide_version)
            </py-script>
        """,
            extra_head="""
            <py-config>
                runtimes:
                - src: "/pyodide/pyodide.js"
                  name: pyodide-0.20.0
                  lang: python
            </py-config>
        """,
        )

        assert self.console.log.lines == [self.PY_COMPLETE, "version 0.20.0"]
        version = self.page.locator("py-script").inner_text()
        assert version == "0.20.0" 

Playing spot the difference, I've realized one file had pyconfig as YAML the other one as JSON, so I changed that on my last commit (2cfe470) and it worked. But I wonder:
a) why is this failing right here, right now? Seems so unrelated to what I'm doing
b) why isn't the YAML config working on this scenario?

Help here would be appreciated cause I don't understand what's happening.

@madhur-tandon

Copy link
Copy Markdown
Contributor

@marimeireles That's because YAML is no longer supported. Only TOML and JSON are.

@marimeireles

Copy link
Copy Markdown
Contributor Author

So it's fine to keep it as it is?

@tedpatrick

tedpatrick commented Oct 14, 2022 via email

Copy link
Copy Markdown
Contributor

@madhur-tandon

Copy link
Copy Markdown
Contributor

So it's fine to keep it as it is?

No no, it means that you have old history, the repo doesn't have YAML anywhere now. So you should ideally rebase on top of main and force push.

@@ -0,0 +1,71 @@
import os

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the presence of this file is wrong and it's probably a merge artifact.
It was renamed into test_py_config.py by PR 806, see here:
https://github.com/pyscript/pyscript/pull/806/files#diff-be855b518404836a1b9ed7ccecb56a8055aa1b0a11f3c73743cf44ac18a633bb

You can safely remove it.

@antocuni

Copy link
Copy Markdown
Contributor

If we are seeing YAML in the tests, it implies that the branch history is not current in the PR or commits are missing. We are close but merging this holds risk if the branch history is missing commits.

Mystery solved, it is caused by a merge artifact, because test_py_runtime_config.py was renamed into test_py_config.py, see this comment:
#749 (comment)

@antocuni

antocuni commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

@marimeireles today I merged #850 which caused another conflict in this branch, so I took the freedom to fix it in e1b1045, hope you don't mind.
I also killed test_py_runtime_config.py in 0a563b2.

I think we are very close to merge it! :)

@marimeireles

Copy link
Copy Markdown
Contributor Author

@antocuni got it! 🎉
Thank you! :)
okaaaay I just need one more change.
And then we can merge it.
Pushing it soon.

@marimeireles

Copy link
Copy Markdown
Contributor Author

Ready to merge when the check is green ✅

@tedpatrick tedpatrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome work @marimeireles 🚢

@antocuni antocuni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, good job @marimeireles 🎉🎉🎉

@antocuni

Copy link
Copy Markdown
Contributor

@marimeireles please please please do not use the default commit message that it's suggested when you do "squash and merge": it's completely useless since it's just the concatenation of individual commits.
What I usually do is to write a summary of what the PR does, so that it will be easy to read it when we do git log from the terminal. Often it's a copy&paste&adapt of the first message of my PRs.
See e.g. the commits beb3aa1 and 11a517b as examples

@marimeireles marimeireles merged commit 1587273 into pyscript:main Oct 17, 2022
@marimeireles

Copy link
Copy Markdown
Contributor Author

Docs follow soon.

JeffersGlass pushed a commit to JeffersGlass/pyscript that referenced this pull request Oct 17, 2022
…rowser console (pyscript#749)

* Add display impl, remove outputManage, print and console.log defaults to terminal

* Fixing tests

* Lint

* Erase unecessary code, add cuter CSS formating for errors, fix problems around REPL output

* Add fix to repl2 and lint

* lint

* Allow for list of display, fix elif to else

* Add better global option

* test work

* xfails

* (antocuni, mariana): let's try to start again with TDD methodology: write the minimum test and code for a simple display()

* (antocuni, mariana): this test works out of the box

* WIP: this test is broken, mariana is going to fix it

* add a failing test

* Add ability to deal with targets

* Add append arg and append tests

* Add multiple values to display

* Small adjustments to tests. I noticed I wasn;t running all at some point

* add display test

* Add console tests

* Add async tests

* Fix repl tests

* Fixing merging issues

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address antocuni's review

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fixing more tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* linting

* Improve repl tests

* Change my test so codespell is hapy with it

* Test: change test_runtime_config to use json instead of toml to see if stops failing on CI

* kill this file: it is a merge artifact since it was renamed into test_py_config.py on the main branch

* Change test execution order and add async tests to async test file

Co-authored-by: Antonio Cuni <anto.cuni@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
antocuni added a commit that referenced this pull request Oct 27, 2022
Major highlights:

1. Merge&simplify base.ts and pyrepl.ts; kill base.ts
2. improve and extente the py-repl integration tests
3. Reorder the code in pyrepl.ts. This part of the PR doesn't change much of the concrete logic: it's just a sequence of renaming variables, moving code around, group code into functions, killing code which is no longer needed. But the end result is much better and nicer to read, IMHO.

Minor highlights:

1. py-repl now uses the new logic in pyexec.ts to run the code

2. after PR Add display impl, rm outputManage, print and console.log default to browser console #749 py-repl no longer displayed the result of the last evaluated expression (e.g. if you typed 42 and run it, it displayed nothing). This PR re-introduces this behavior, which is what you would expect by a REPL.

3. improve the pytest --dev option: now it implies --no-fake-server so that sourcemaps works automatically

4. improve the names of the CSS classes to be more consistent

5. kill pyrepl.test.ts: the old tests didn't check anything useful,  this style of unit test doesn't really add much value if you have good integration tests (which now we have) and trying to revive them was not worth the hassle
JeffersGlass pushed a commit to JeffersGlass/pyscript that referenced this pull request Oct 31, 2022
…rowser console (pyscript#749)

* Add display impl, remove outputManage, print and console.log defaults to terminal

* Fixing tests

* Lint

* Erase unecessary code, add cuter CSS formating for errors, fix problems around REPL output

* Add fix to repl2 and lint

* lint

* Allow for list of display, fix elif to else

* Add better global option

* test work

* xfails

* (antocuni, mariana): let's try to start again with TDD methodology: write the minimum test and code for a simple display()

* (antocuni, mariana): this test works out of the box

* WIP: this test is broken, mariana is going to fix it

* add a failing test

* Add ability to deal with targets

* Add append arg and append tests

* Add multiple values to display

* Small adjustments to tests. I noticed I wasn;t running all at some point

* add display test

* Add console tests

* Add async tests

* Fix repl tests

* Fixing merging issues

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address antocuni's review

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fixing more tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* linting

* Improve repl tests

* Change my test so codespell is hapy with it

* Test: change test_runtime_config to use json instead of toml to see if stops failing on CI

* kill this file: it is a merge artifact since it was renamed into test_py_config.py on the main branch

* Change test execution order and add async tests to async test file

Co-authored-by: Antonio Cuni <anto.cuni@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
JeffersGlass pushed a commit to JeffersGlass/pyscript that referenced this pull request Nov 14, 2022
…rowser console (pyscript#749)

* Add display impl, remove outputManage, print and console.log defaults to terminal

* Fixing tests

* Lint

* Erase unecessary code, add cuter CSS formating for errors, fix problems around REPL output

* Add fix to repl2 and lint

* lint

* Allow for list of display, fix elif to else

* Add better global option

* test work

* xfails

* (antocuni, mariana): let's try to start again with TDD methodology: write the minimum test and code for a simple display()

* (antocuni, mariana): this test works out of the box

* WIP: this test is broken, mariana is going to fix it

* add a failing test

* Add ability to deal with targets

* Add append arg and append tests

* Add multiple values to display

* Small adjustments to tests. I noticed I wasn;t running all at some point

* add display test

* Add console tests

* Add async tests

* Fix repl tests

* Fixing merging issues

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address antocuni's review

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fixing more tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* linting

* Improve repl tests

* Change my test so codespell is hapy with it

* Test: change test_runtime_config to use json instead of toml to see if stops failing on CI

* kill this file: it is a merge artifact since it was renamed into test_py_config.py on the main branch

* Change test execution order and add async tests to async test file

Co-authored-by: Antonio Cuni <anto.cuni@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[622] py-script should only render to the page through the display function

7 participants