Simplify pyrepl.ts and kill base.ts by antocuni · Pull Request #884 · pyscript/pyscript

antocuni · 2022-10-24T14:58:38Z

Yet another of my refactorings. This PR does two things which are related but also clearly separated, and probably it would have been better to make two different ones, but too late now.

I suggest reviewers to review the two parts separately, it is probably easier to understand what I did:

Merge&simplify base.ts and pyreply.ts in the first 25 commits (from 9c225b2 to 0c661f5)

BaseEvalElement was used only by pyrepl now. By merging them I could simplify a lot the logic and kill a lot of code. In the process, I also improved and extented the py-repl integration tests, to ensure that I didn't break anything.

Reorder the code in pyrepl.ts in the next 29 commits (from 0a9e19d to the end)

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 (you might also want to look at the commits one by one, every of them is self-contained and passed the tests). But the end result is much better and nicer to read, IMHO.

Other highlights:

py-repl now uses the new logic in pyexec.ts to run the code
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.
I improved the pytest --dev option: now it implies --no-fake-server so that sourcemaps works automatically
many many tests but the autogenerate functionality is untested. It should probably be improved/refactored by another PR
I improved the names of the CSS classes to be more consistent

A possibly controversial change:

I killed pyrepl.test.ts: the old tests didn't check anything useful, just that some of the attributes existed on the instance, but after the refactoring I killed most of them and they were impossible to test in that way. I claim that 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. This is the relevant commit: 8e8ed9c

…inside evaluate

…e' anyway

…UniqueId earlier

We want to run the most basic tests first: so first we check that we can print(), and only later we test the behavior of display(), which is much more complex. Also, add a test to ensure that we can load&execute code which was typed inside the <py-repl> This simplifies the rest of the tests because we can put the code directly in the tag instead of having to use .type() all the times

…t. Write a test and its fix

…we format it incorrectly

…ated expression, as you would expect by a REPL

…he output ID doesn't exist

…e old code, they are too hard to fix and they were not really testing anything useful

fpliger · 2022-10-25T20:54:01Z

-            editorLabel.setAttribute('style', labelStyle);
-            editorLabel.htmlFor = 'code-editor';
+            boxDiv.append(editorLabel);
+            boxDiv.appendChild(editorDiv);


So, this means we always create the output div, even if we have an explicit output attribute?

yes, I did it intentionally because it simplify handling the case in which output=... points to a non-existent ID:

pyscript/pyscriptjs/src/components/pyrepl.ts

Lines 187 to 191 in 02ca665

if (el === null) {

const err = `py-repl ERROR: cannot find the output element #${outputID} in the DOM`

this.outDiv.innerText = err;

return undefined;

}

I think that an empty div should not cause problems, does it?

I don't think it does and I'm probably too picky here. It just leaves an unused empty tag which I don't think is a big deal.

fpliger · 2022-10-25T21:06:03Z

I know this PR is WIP but thought I'd leave early feedback. Great work ;)

antocuni · 2022-10-26T08:54:24Z

I know this PR is WIP but thought I'd leave early feedback. Great work ;)

actually when you reviewed it it was almost ready, so you saw basically everything. But see the top message now, it's probably worth to look at two sets of commits separately.

marimeireles · 2022-10-26T15:30:01Z

@@ -8,61 +8,78 @@ class TestPyRepl(PyScriptTest):
    def test_repl_loads(self):


This entire file looks good to me 👍
Should I review the rest too?

This entire file looks good to me

🎉

Should I review the rest too?

reviews are always welcome :)

tedpatrick · 2022-10-26T23:38:04Z

solid

fpliger

🚀 🚀 🚀 🚀

Co-authored-by: woxtu <woxtup@gmail.com>

…hich was broken by PR #884

What is not tested is broken man_facepalming Test&fix <py-repl auto-generate=true> which was broken by PR #884

antocuni added 13 commits October 24, 2022 16:26

mechanically move evaluate from base.ts into pyrepl.ts

9c225b2

mechanically kill {pre,post}Evaluate and inline (some) of their code …

f87f3dc

…inside evaluate

kill appendOutput and setOutputMode, since pyrepl always used 'replac…

f14e542

…e' anyway

kill more methods from base.ts

1a772cf

move the ctor logic from BaseEvalElement to PyRepl

40a2154

move the last remaining bits from base.ts to pyrepl.ts

77d8889

finally kill base.ts 🎉

8fca125

getEditorTheme can be a method now

4696846

kill errorElement. It's always the same as outputElement anyway

9c2dea0

kill this warning. It's impossible to trigger it since we call ensure…

1ff2273

…UniqueId earlier

kill this as well

9976ae3

fix pyrepl unit test

c602f60

FabioRosado mentioned this pull request Oct 24, 2022

Add more type definitions #882

Merged

antocuni added 10 commits October 25, 2022 00:31

a display() inside py-repl should send the output to its outputElemen…

ed52ef0

…t. Write a test and its fix

WIP: improve the test about exceptions, but it's failing now because …

10e0459

…we format it incorrectly

kill kill kill a lot of code and simplify stuff

debb33e

check that the exception is also written to the console

02f98d3

make sure to clear the old output before executing new code

6e116d9

improve this test

8a16bb0

rename and improve this test

7fb0170

we don't need to re-locate the out div again and again

6aa8a47

re-enable the functionality to automatically display() the last evalu…

866210d

…ated expression, as you would expect by a REPL

add tests for the output attribute, and a fix for the case in which t…

c178b92

…he output ID doesn't exist

madhur-tandon mentioned this pull request Oct 25, 2022

Format the TypeScript files #877

Merged

antocuni added 5 commits October 25, 2022 14:08

improve --dev, now it also implies --no-fake-server

d7f2299

this line is not needed, it's a duplicate

0c661f5

start to document the DOM of py-repl, and rename a variable

0a9e19d

rename editorNode into editorDiv

962c889

s/mainDiv/boxDiv

2884402

antocuni added 7 commits October 25, 2022 20:40

improve the comment, move code around

d5da119

this.code doesn't have to be a member

6aa7e5a

rename

09b9232

simplify the code

480ac0e

add a comment

671ba3e

kill the pyrepl unit tests: they relied on the internal details of th…

8e8ed9c

…e old code, they are too hard to fix and they were not really testing anything useful

kill unused imports

02ca665

fpliger reviewed Oct 25, 2022

View reviewed changes

Comment thread pyscriptjs/src/components/pyrepl.ts

fpliger reviewed Oct 25, 2022

View reviewed changes

Comment thread pyscriptjs/src/components/pyrepl.ts

fpliger reviewed Oct 25, 2022

View reviewed changes

use a different strategy

38169f6

antocuni changed the title ~~WIP: Merge&simplify base.ts and pyrepl.ts~~ Simplify pyrepl.ts and kill base.ts Oct 26, 2022

antocuni marked this pull request as ready for review October 26, 2022 08:53

antocuni requested review from fpliger and tedpatrick October 26, 2022 08:54

marimeireles reviewed Oct 26, 2022

View reviewed changes

tedpatrick approved these changes Oct 26, 2022

View reviewed changes

woxtu reviewed Oct 27, 2022

View reviewed changes

Comment thread pyscriptjs/src/components/pyrepl.ts Outdated

fpliger approved these changes Oct 27, 2022

View reviewed changes

This was referenced Oct 27, 2022

py-repl doesn't support loading code with a src attribute #892

Closed

PyScript should support more than on py-repl per page #893

Closed

typo

b3d0387

Co-authored-by: woxtu <woxtup@gmail.com>

antocuni merged commit 214e395 into main Oct 27, 2022

antocuni deleted the antocuni/kill-base-ts branch October 27, 2022 08:10

antocuni added a commit that referenced this pull request Oct 31, 2022

What is not tested is broken. Test&fix <py-repl auto-generate=true> w…

ddfcac5

…hich was broken by PR #884

antocuni mentioned this pull request Oct 31, 2022

Fix py-repl auto-generate=true #910

Merged

antocuni added a commit that referenced this pull request Oct 31, 2022

Fix py-repl auto-generate=true (#910)

f67b8e0

What is not tested is broken man_facepalming Test&fix <py-repl auto-generate=true> which was broken by PR #884

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify pyrepl.ts and kill base.ts#884

Simplify pyrepl.ts and kill base.ts#884
antocuni merged 55 commits into
mainfrom
antocuni/kill-base-ts

antocuni commented Oct 24, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

fpliger Oct 25, 2022

Uh oh!

antocuni Oct 25, 2022

Uh oh!

fpliger Oct 27, 2022

Uh oh!

fpliger commented Oct 25, 2022

Uh oh!

antocuni commented Oct 26, 2022

Uh oh!

marimeireles Oct 26, 2022

Uh oh!

antocuni Oct 26, 2022 •

edited

Loading

Uh oh!

tedpatrick commented Oct 26, 2022

Uh oh!

Uh oh!

fpliger left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if (el === null) {
	const err = `py-repl ERROR: cannot find the output element #${outputID} in the DOM`
	this.outDiv.innerText = err;
	return undefined;
	}

		@@ -8,61 +8,78 @@ class TestPyRepl(PyScriptTest):
		def test_repl_loads(self):

Conversation

antocuni commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fpliger Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

antocuni Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

fpliger Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

fpliger commented Oct 25, 2022

Uh oh!

antocuni commented Oct 26, 2022

Uh oh!

marimeireles Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

antocuni Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tedpatrick commented Oct 26, 2022

Uh oh!

Uh oh!

fpliger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

antocuni commented Oct 24, 2022 •

edited

Loading

antocuni Oct 26, 2022 •

edited

Loading