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

Allow `set` plugin can now set entry fields to types other than str #2344

Merged
merged 3 commits into from Feb 21, 2019

Conversation

Projects
None yet
4 participants
@gazpachoking
Copy link
Member

gazpachoking commented Feb 19, 2019

Motivation for changes:

If you want to manipulate a number field it is not currently possible.

Detailed changes:

  • If set plugin templates evaluate to a type other than string, the type is not cast to a string.

Addressed issues:

Config usage if relevant (new plugin or updated schema):

set:
  some_field: {{some_int_field/2}}

To Do:

  • Should a 'native' parameter be added to the Entry.render method rather than constructing the NativeTemplate in set plugin?

gazpachoking added some commits Feb 19, 2019

Add 'native' parameter to Entry.render to invoke jinja native rendering
Remove some uses of 'basestring' (which we should not need anywhere)
@gazpachoking

This comment has been minimized.

Copy link
Member Author

gazpachoking commented Feb 20, 2019

So, I'm generally a fan of this 'native' rendering for the way we are using jinja templates. I think it allows more user manipulation of entry fields with the only cost being quotes sometimes needed if you explicitly need a string. @paranoidi @liiight @cvium Anything I'm overlooking?

@@ -34,7 +33,7 @@ def modify(self, entry, config, errors=True):
"""This can be called from a plugin to add set values to an entry"""
for field in config:
# If this doesn't appear to be a jinja template, just set it right away.
if not isinstance(config[field], basestring) or '{' not in config[field]:
if not isinstance(config[field], str) or '{' not in config[field]:

This comment has been minimized.

@paranoidi

paranoidi Feb 20, 2019

Member

Doesn't this remove unicode support? Why swtich from basestring -> str, we are not yet python3? :)

This comment has been minimized.

@cvium

cvium Feb 20, 2019

Member

It's a redundant import actually. Because of from builtins import *, str is equivalent to basestring.

Source: https://python-future.org/compatible_idioms.html#basestring

This comment has been minimized.

@gazpachoking

gazpachoking Feb 20, 2019

Author Member

Actually, 'str' is equivalent to 'unicode' on python 2 (because of the from builtins import *.) We don't actually allow byte strings in any of these places.

@liiight
Copy link
Member

liiight left a comment

Looks good @gazpachoking. Maybe add native types specific tests?

@gazpachoking

This comment has been minimized.

Copy link
Member Author

gazpachoking commented Feb 20, 2019

@liiight Yeah, good plan, added.

@gazpachoking gazpachoking merged commit 27928b0 into develop Feb 21, 2019

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: test-py27 Your tests passed on CircleCI!
Details
ci/circleci: test-py34 Your tests passed on CircleCI!
Details
ci/circleci: test-py35 Your tests passed on CircleCI!
Details
ci/circleci: test-py36 Your tests passed on CircleCI!
Details
ci/circleci: test-py37 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.