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

Suggested patch for a shorthand definition of quoted fields in http_urllib #693

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

otdftr
Copy link
Contributor

@otdftr otdftr commented Feb 8, 2024

When working with a greater number of transformed fields (e. g. through topic's alldata), the params definition for the target has a lot of redundancy, especially when the field names are crafted to be the parameter names of the query parameters:

This patch provides a shorthand for a definition like
[ #method, #url, { 'param1' : '@param1', ..., 'paramN': '@paramN' }, ...
by allowing lists for the 3rd parameter of the target definition:
[ #method, #url, [ '?param1', ..., '?paramN' ], ...

If a list is provided

  • the given names will be used as both keys do item.data{} and query parameter names
  • they will be added as quoted fields, regardless of whether the @-prefix is provided: [ 'param1', ..., 'paramN' ] will be interpreted as [ '@param1', ..., '@paramN' ]
  • with the prefix ?, fields can be declared optional: they will not be included in the query if the data is invalid or missing from item.data{}; [ 'param1', '?param2', ... ]

add list shorthand for params dict + allow optional params in query string
@otdftr otdftr marked this pull request as ready for review February 8, 2024 16:16
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 49.60%. Comparing base (9d61fc5) to head (675fbb0).

Files Patch % Lines
mqttwarn/services/http_urllib.py 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
- Coverage   49.75%   49.60%   -0.15%     
==========================================
  Files          81       81              
  Lines        4034     4046      +12     
==========================================
  Hits         2007     2007              
- Misses       2027     2039      +12     
Flag Coverage Δ
unittests 49.60% <0.00%> (-0.15%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amotl
Copy link
Member

amotl commented Feb 14, 2024

Dear Olaf,

thanks for your contribution, I like it. While I don't have any objections about merging this improvement, I would like to have @jpmens and @sumnerboy12 the final voice on it.

With kind regards,
Andreas.

Copy link
Collaborator

@jpmens jpmens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

@amotl amotl self-assigned this Mar 27, 2024
@amotl
Copy link
Member

amotl commented May 8, 2024

I think this can be merged. However, would it be possible to come up with corresponding test cases so the new code and functionality will be covered properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants