search

matt-kempster / m2c

A MIPS and PowerPC decompiler.
GNU General Public License v3.0
404 stars 49 forks source link

Package for use as a library #246

Closed bates64 closed 1 year ago

bates64 commented 1 year ago
matt-kempster commented 1 year ago

Looks good to me. Could we add some documentation to README.md as well?

simonlindholm commented 1 year ago

thoughts:

simonlindholm commented 1 year ago
bates64 commented 1 year ago

is there a way to make this work with src/, or is it needed to get m2c.main and similar import paths?

I'm pretty certain its required - if we make it src then you have to do import src.

does mypy type-checking still work for decomp.me with this change, for calls into m2c?

AFAIK, yes, but I will double-check

is 0.1.0 the right version to indicate that aren't explicitly versioning m2c?

Unsure, but a version is required

license: "GPL-3.0" is probably more right than "GPL-3.0-or-later"

Will update to GPL-3.0-only if that's more correct

using m2c as a library is incredibly niche, we shouldn't put that towards the top of the README

Where else do you suggest?

simonlindholm commented 1 year ago

Unsure, but a version is required

I guess it doesn't actually matter, we can keep the 0.1.

Where else do you suggest?

Hm, good question, there doesn't seem to be a clear spot. Honestly I'd kinda be inclined to leave it out altogether.

Another point: we may want to add pre-commit, mypy, coverage, black==22.3.0 to dev-dependencies, and graphviz to dependencies.

matt-kempster commented 1 year ago

You can relegate the poetry documentation to the very bottom of the README (or near it). I agree that it doesn't deserve the real-estate up top.

bates64 commented 1 year ago

OK, I've made the requested changes

bates64 commented 1 year ago

Also- to check that the pyproject.toml dependencies are actually valid, a2429798129b8b478f6b5b4b676250e15eb8f97b updates CI to use the Poetry virtualenv for tests/mypy/black. I need approval to check the workflow runs correctly, though!

simonlindholm commented 1 year ago

Also- to check that the pyproject.toml dependencies are actually valid, https://github.com/matt-kempster/m2c/commit/a2429798129b8b478f6b5b4b676250e15eb8f97b updates CI to use the Poetry virtualenv for tests/mypy/black. I need approval to check the workflow runs correctly, though!

Nice! I clicked the "run workflow" button, but I think the yaml still gets taken from the master branch, not from the PR, for security reasons, so it may need the PR to be merged and iterated on in follow-up PRs in case it fails (which is fine). Or one could use some kind of "run GHA locally" tool to check it, I guess.

simonlindholm commented 1 year ago

hm, it does look like the updated yaml ran though, and failed

simonlindholm commented 1 year ago

does mypy type-checking still work for decomp.me with this change, for calls into m2c?

AFAIK, yes, but I will double-check

Apparently it did not, you needed a py.typed file for it to work (noticed this when looking at a silent splat/spimdisasm type-checking failure). Added in b52d92c2340f6f4ba1aafb464188bb698752fbb0.