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

Added cli_serializer & from_cli function #311

Merged
merged 3 commits into from
Jul 15, 2023

Conversation

Denperidge
Copy link
Contributor

@Denperidge Denperidge commented Jul 6, 2023


name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo


Describe your changes

  • Implemented a from_cli function and its respective serializer + tests

Related issue/discussion
#290

Extra notes:

  • ArgumentParser allows for an auto-generated --help interface, but this not possible in the current implementation. Support for this could be added, but it would require extra top level functions, and I am unsure as to whether that would be overkill or out of scope
  • It should be possible to allow from_cli to take 0 arguments, as the internally used parse_args function also allows 0-1 arguments (in case of 0, it takes the arguments passed to the currently running script). However, I don't know if that behaviour is desirable, as it would make from_cli more unlike the other from_* functions
  • Technically, to_cli could also be possible (e.g. create a string with the command that would need to be run to get these options), but I don't know if that has much use

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests for the proposed changes.
  • I have run the tests and there are not errors.

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Jul 6, 2023

@Denperidge thank you very much for this PR!

I see there are some errors on Python 3.8, could you fix them?
I will review this as soon as possible.

@fabiocaccamo fabiocaccamo self-requested a review July 6, 2023 16:30
@fabiocaccamo fabiocaccamo added the enhancement New feature or request label Jul 6, 2023
@Denperidge
Copy link
Contributor Author

My pleasure!

Got the error! The exit_on_error option got added in 3.9. But because of the error catching that is done further down, it should be redundant either way.

Take your time to review, and have a nice day!

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09 🎉

Comparison is base (83e4853) 96.44% compared to head (a0e356f) 96.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   96.44%   96.54%   +0.09%     
==========================================
  Files          61       62       +1     
  Lines        1857     1909      +52     
==========================================
+ Hits         1791     1843      +52     
  Misses         66       66              
Flag Coverage Δ
unittests 96.54% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
benedict/dicts/io/io_dict.py 100.00% <100.00%> (ø)
benedict/serializers/__init__.py 100.00% <100.00%> (ø)
benedict/serializers/cli.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

benedict/serializers/cli.py Outdated Show resolved Hide resolved
benedict/serializers/cli.py Outdated Show resolved Hide resolved
@fabiocaccamo fabiocaccamo merged commit 1feb11c into fabiocaccamo:main Jul 15, 2023
@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Jul 16, 2023

@Denperidge released within 0.32.0 version.

@Denperidge
Copy link
Contributor Author

Amazing, thanks for helping me integrate it! @fabiocaccamo Have a nice day ♥

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Jul 18, 2023

Thank you for your great contribution!
Have a nice day you too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants