Skip to content
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

request for --yaml option #37

Closed
cbm755 opened this issue Feb 13, 2023 · 2 comments · Fixed by #41
Closed

request for --yaml option #37

cbm755 opened this issue Feb 13, 2023 · 2 comments · Fixed by #41
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@cbm755
Copy link
Collaborator

cbm755 commented Feb 13, 2023

Low priority and I guess I don't have a strong argument for yaml, except that flatpak-pip-generator has it (also via --yaml) and I prefer reading yaml over JSON.

(Thanks for this project, works great and will save me a lot of time!)

@real-yfprojects real-yfprojects added enhancement New feature or request good first issue Good for newcomers labels Feb 18, 2023
@johannesjh
Copy link
Owner

johannesjh commented Feb 18, 2023

hi Colin, thank you for your positive feedback and thank you for this suggestion. Would you want to submit this as pull request?

Some design considerations / suggestions:

  • poetry dependencies: the pyyaml dependency should be optional, i.e., an "extra" dependency in poetry.
  • commandline option: you could add a --format or -f commandline parameter, defaulting to json, optionally yaml.
  • handling import errors: the script should still be able to produce json, even if pyyaml has not been installed. you could import pyyaml locally, i.e. in the method where you need it instead of at the beginning of the file. you could wrap the import statement in a try/catch block.
  • yaml serialization: req2flatpak currently has only one method build_module_as_str which serializes to a json string. you could create two different methods instead, one for json and another one for yaml serialization.
  • testing: a test case to make sure that yaml output is produced
  • documentation: a section in cli.rst to describe the new commandline option

@cbm755
Copy link
Collaborator Author

cbm755 commented Feb 21, 2023

I might have a chance to take a look next time I'm bumping deps on my flatpak.

Your step-by-step looks very well-thought out: I agree "good first issue" for someone looking to get involved: I'll certainly not be offended if someone does it first, and will be happy to test.

cbm755 added a commit to cbm755/req2flatpak that referenced this issue Feb 25, 2023
@cbm755 cbm755 mentioned this issue Feb 25, 2023
9 tasks
@johannesjh johannesjh added this to the v0.2 milestone Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants