Conversation
fabi1cazenave
left a comment
fabi1cazenave
left a comment
There was a problem hiding this comment.
Thanks for this great work ! It works, it’s well written and it conforms perfectly to the existing (and debatable) logic. I love it. :-)
| import click | ||
| from lxml import etree | ||
| from lxml.builder import E | ||
|
|
There was a problem hiding this comment.
these imports might be unused
There was a problem hiding this comment.
right. My linter agrees :)
| "black", | ||
| "isort" | ||
| ] | ||
|
|
There was a problem hiding this comment.
I’m nowhere near a Python expert, but ain’t there some kind of tool.dev-depencies magic we could use here instead ? Where tool != poetry or anything that would pull more dependencies than Kalamine itself ?
There was a problem hiding this comment.
Without using additional dependency manager tool, that's it. You define optional deps.
With an external tool, well, poetry.
By the way, I modified the Makefile as you directly called kalamine which was installed in my python. As I can't understand if you use virtualenvs for tests, I simply used python -m kalamine.cli not to cath the installed kalamine executable. That could be the subject of another PR, but again, I need to uderstand what kind of system-wide tests you could do (By the way, the tests are broken :s).
There was a problem hiding this comment.
The tests work… if you call them with a make test, because they expect the layouts to be built. I know it’s not how it’s supposed to work — if you can think of a better way and include that in the CI, your help would be very welcome.
There was a problem hiding this comment.
Ok. I understand. Tests import again my system-wide kalamine.
|
|
||
| if TYPE_CHECKING: | ||
| from .layout import KeyboardLayout | ||
|
|
There was a problem hiding this comment.
Oh I’d love to have type checking everywhere. <3
Does that affect the minimum python version we support (= 3.6 at the moment) ?
There was a problem hiding this comment.
And I simply added one or two annotations I had to identify to implement the PR. This is great for developers.
There was a problem hiding this comment.
Agreed. I love type annotations and I’ve just found out about pathlib very recently (it’s just used in a couple files). Your PRs would help a lot.
kalamine/layout.py
Outdated
| f"svg:g/svg:text[@class='level{level_num}']", namespaces=ns | ||
| ): | ||
| if char not in deadkeys: | ||
| # Not a deadkey |
There was a problem hiding this comment.
nitpick, we can safely drop this comment :-)
|
Fixes #54. |
2 participants
This MR adds SVG rending as a new file output. This is generated as any other file type.
To test it, simply run (e.g. for
ergol.tomlfile)To test all rending classes, add
dk,altgr,mixedclasses to mainsvgtag to display resp. 1st deadkey, altgr and a mixed view layer.