kervinck / gigatron-rom

System, apps and tooling for the Gigatron TTL microcomputer
BSD 2-Clause "Simplified" License
235 stars 80 forks source link

121 migrate from python 2 to 3 #124

Closed TeddyBear06 closed 4 years ago

TeddyBear06 commented 4 years ago

From my perspective, it seems pretty ready for Python 3 now, all scripts seems to be fully functionnal (but I still need your review to be 100% sure).

Mainly strings formatters changes.

I did add a verification (line 338 of Core/gcl0x.py) due to Python 3 byte type handling.

I did run tests successfully (both make test and make compiletest). I diff compiletest outputs from master version Python 2 and my branch version Python 3, they are strictly identicals.

I created a .gitignore to avoid building artefacts to be tracked.

Please let me know if you need something else.

Kind regards,

kervinck commented 4 years ago

Oh wow, that more work than I expected. I wasn't aware that they removed the format operator % as well! Give me 2 or 3 days to look it all through.

TeddyBear06 commented 4 years ago

Well, the % format operator is not dropped, it's the "old way" of formatting strings in Python 3 (cf. https://docs.python.org/3/tutorial/inputoutput.html#old-string-formatting).

If you prefer, we can keep the old formatting style.

Finally, of course, take all your time to review it mindfully.

kervinck commented 4 years ago

Don't worry, I just have to catch up with the things p3 bring. Is there some guidance from Guido on which method to use?

TeddyBear06 commented 4 years ago

Well, Guido said that the .format() method is quite verbose and "suggest" to use f-strings in "rescue" (source PyCascades 2018).

Screenshot : https://i.imgur.com/gdteM3Q_d.jpg?maxwidth=2000 Full video : https://youtu.be/Oiw23yfqQy8

Yet less verbose, It still need a lot - deeper - more modifications.

For example (https://github.com/kervinck/gigatron-rom/blob/master/Core/ROMv1.py#L238) :

We can follow is point of view if you prefer, I'm really open and it's a matter of choice.

But I still think .format() is an explicit, familiar and robust way of formatting strings :-)

kervinck commented 4 years ago

Ok clear. Does he have an argument against the % operator? It's most concise and suitable for the job?

kervinck commented 4 years ago

Half year ago I struggled with the LCC port because brew didn't have python 3.6 yet. But now this is is solved. f-strings are indeed better than ''.format(). But I don't know what's the problem with %. The only issue is that python2 will not be supported from 2020 on, so I was expecting only a couple print statements needed brackets.

TeddyBear06 commented 4 years ago

Ok I see.

But as I said it just a matter of choice (personnaly the "f" of f-strings lost between a parenthesis and a quote is not really readable).

Let me revert my modifications and just fix print parenthesis still using old string format with % and fix some other typing issues to make all scripts works with Python 3 :-)

Is that okay for you ?

kervinck commented 4 years ago

I agree that the f'-modifier isn't the greatest, but I can understand that Guido needed to do something like that. In a simple language that I restarted to design many times over, the '...{expr}...' expansions where the default. Sure I loved it when out of the blue almost the same notation appeared in p3!

To me, ''.format() feels a bit like exposing implementation internals to the user. It is almost shouting out "hey, did you know these strings are really objects that can do this cool trick?". While the %-notation was already ok'ish and kept things it concise. It's certainly also not perfect, but good enough for simple programs.

So if there's no pending removal of this operator in p3, I would vote for keeping it. This is not enterprise software after all. It is best served by keeping visual clutter down.

TeddyBear06 commented 4 years ago

So if there's no pending removal of this operator in p3, I would vote for keeping it. This is not enterprise software after all. It is best served by keeping visual clutter down.

As far as I know, there is no pending removal.

I dig a little more deeper and I found that the django web framework is still using this notation so it's more than enough to convince me :-)

v = '%s… <trimmed %d bytes string>' % (v[0:4096], len(v))

I'm gonna update my pull request.