-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add yaml output #41
Add yaml output #41
Conversation
Start of a fix for Issue johannesjh#37.
Co-authored-by: yfprojects <[email protected]>
Are you using pre-commit? |
update luddite-mode turned off, trying it... |
I think this is for older pyyaml, AFAICT, False is the default circa 2022. No need to specific indent=2 as that seems to be the default.
pre-commit mucked around in
The yaml.safe_load() seems nothing to worry about b/c its in a test. |
now I can't commit, not too impressed so far...
|
... and installing it does not go well:
So back to luddite mode for me I think! |
I'm not very confident about the test: please take a look at that. |
Don't blame me, but I had to smile reading about your struggle. Our pre-commit setup is not prefect yet. I tweaked it in #42 and #39.
You can instal it on a pre-repository basis. |
To fix the ci you will have to run the following: poetry lock --no-update && SKIP=poetry-lock git commit |
Also avoids warnings from linters.
This is ready to go from my point-of-view, but maybe needs some clean-up about testing with optional dependency |
I can add a commit for fixing ci. |
* pyproject.toml (tool.poetry.dependencies): Add `pyyaml` as optional dependency. * poetry.lock : Run `poetry lock --no-update` to add pyyaml.
* pyproject.toml * poetry.lock
92f1ea4
to
72109fd
Compare
* .github/workflows/ci.yml
72109fd
to
de6c5a6
Compare
hi guys, this PR is almost finished, well done! we have a few open comments from the code review and three complaints from pylint. @cbm755 are you going to walk the last mile? I hope you are not getting annoyed, do you need / want help? |
I don't know about the complexity complaint. The import complaints could be resolved similar to how I did it in the tests. |
I would suggest to ignore the complexity constraint.
|
Yaml is tried to be imported else `yaml` is set to `None`. Later the code will only check wether `yaml` is True (is not None). * req2flatpak.py
* req2flatpak.py : Ignore pylint complain `too-many-branches` for `main`.
* tests/test_req2flatpak.py (Req2FlatpakBaseTest.requirements_file): Implement this method returning a contextmanager that creates and prefills a requirements file with requirements. * tests/test_req2flatpak.py (Req2FlatpakBaseTest.test_cli_with_reqs_as_file[_yaml]): Use `requirements_file()`.
These methods implement formatting, so why duplicate that code. Fixes johannesjh#45. * req2flatpak.py (main)
Thanks @real-yfprojects for the edits. I tested this and it works for me! I do notice that the yaml dict out is in a rather unpleasant order:
Note "build-commands" before "name" for example. And "sha256" before "type". Its still valid but I wonder why that happens? Should I file an new issue? For comparison, here is the JSON:
My understanding of modern Python is that dicts have a consistent unofficial ordering based on the the order you insert things into them. In this case, that translates into a pleasant ordering when outputing JSON but not YAML... |
I found a very long thread about it here where I discovered there is a |
Good call! |
well done, thank you for the effort! ready to merge. 🎉 |
Fixes #37
todo
Filed classmethod build_module_as_str is untested? #45 for this.build_module_as_str
seems unused and untested, same with the newbuild_module_as_yaml_str
.pyyaml
dep, not at top--yaml
flag--platform-info
should respect--yaml
flaghelp wanted