String Formatting Updated #141

vkmrishad · 2018-10-10T18:54:09Z

BTW I have formatted string in the entire project.

joshcanhelp

Thank you for the contribution here @vkmrishad! Is there a specific reason for this change, like better performance?

vkmrishad · 2018-10-23T03:03:41Z

hi @joshcanhelp,

I can list some benefits of formatting:

Readability. The format string syntax is more readable, as it separates style from the data. Also, in Python, %s syntax will automatically coerce any non str types to str; while concatenation only works with str, and you can't concatenate str with int.
Performance. In Python str is immutable, so the left and right string have to be copied into the new string for every pair of concatenation. If you concatenate four strings of length 10, you will be copying (10+10) + ((10+10)+10) + (((10+10)+10)+10) = 90 characters, instead of just 40 characters. And things get quadratically worse as the number and size of the string increases. Java optimizes this case some of the times by transforming the series of concatenation to use StringBuilder, but CPython doesn't.
For some use cases, the logging library provides an API that uses the format string to create the log entry string lazily (logging.info("blah: %s", 4)). This is great for improved performance if the logging library decided that the current log entry will be discarded by a log filter, so it doesn't need to format the string.

joshcanhelp · 2018-10-23T14:51:54Z

Great response @vkmrishad, thank you! We'll get this reviewed ASAP 👍

vkmrishad · 2018-10-29T16:43:10Z

@lbalmaceda and @joshcanhelp
Please review and merge this PR.

joshcanhelp · 2018-10-29T17:04:31Z

@vkmrishad - We'll get to it when we can. This is not a bug or a critical fix so it's not high priority. Thanks again for your contribution and we'll take a look when we can.

vkmrishad · 2018-10-30T05:06:59Z

@joshcanhelp
Yes, you are right. But after a while this PR will be useless due to conflict. I think its better to close this PR and I can reopen this with new changes when you are ready.

Thanks.

joshcanhelp · 2018-10-30T19:12:46Z

@vkmrishad - Understood. I'd prefer to keep it open so we don't lose track of it but will leave that up to you.

vkmrishad · 2018-10-31T00:50:37Z

@joshcanhelp
Sure, I am keeping this open

joshcanhelp

@vkmrishad - I have one change request on this one and then we'll merge this ... one additional question though ... is these the extent of where this string formatting can be improved? I'm seeing another type of string formatting here:

https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/users.py#L154

Could that be updated as well? Would be helpful to get all these in one PR, if you're willing/able.

Thanks again! I'll stick with this until we get it merged.

joshcanhelp · 2018-11-05T18:43:12Z

auth0/v3/management/connections.py

@@ -128,5 +128,4 @@ def delete_user_by_email(self, id, email):
        Returns:
            An empty dict.
        """
-        return self.client.delete(self._url(id) + '/users', params={'email': email})
-
+        return self.client.delete('%s/users' % self._url(id), params={'email': email})


@vkmrishad - This jumped out at us ... this is not a very clear way to represent that URL. I would change this to do what we're doing here:

https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/users.py#L169

@joshcanhelp
For your information %s formatting is soon to be deprecated. Actually, I thought to use placeholder {} formatting or f-string formation.
I used %s because is mostly used in this project.

If its ok, we can change all formatting from %s to "{}.format()"

vkmrishad · 2018-11-06T14:40:29Z

@joshcanhelp @lbalmaceda

joshcanhelp · 2018-11-06T15:46:51Z

@vkmrishad - No need to tag us, we get notifications 😄

I think this is a big improvement and thank you for sticking with it. We'll review it ASAP and let you know if we have any feedback here.

vkmrishad · 2018-11-06T15:55:30Z

@joshcanhelp I know 😄

joshcanhelp

@vkmrishad - 2 minor things here and we'll get this merged. Both are open to discussion, just ones that jumped out at me.

Thank you!

joshcanhelp · 2018-11-06T21:17:24Z

auth0/v3/management/blacklists.py

-        params = {
-            'aud': aud
-        }
+        self.aud_ = {'aud': aud}


Out of scope, let's leave this out.

Code reverted

joshcanhelp · 2018-11-06T21:20:25Z

auth0/v3/management/connections.py

@@ -138,5 +138,4 @@ def delete_user_by_email(self, id, email):
        Returns:
            An empty dict.
        """
-        return self.client.delete(self._url(id) + '/users', params={'email': email})
-
+        return self.client.delete('{}/users'.format(self._url(id)), params={'email': email})


This is confusing to read ... how about:

return self.client.delete( self._url( '{}/users'.format( id ) ) ...

@joshcanhelp
return self.client.delete( self._url( '{}/users'.format( id ) ) ...
Only 'id' is expected in self._url(id) in this method. Above mentioned code passing both id+'/users'.
This will cause problem to

def _url(self, id=None): url = 'https://{}/api/v2/connections'.format(self.domain) if id is not None: return '{}/{}'.format(url, id) return url

For readability we can change this to
return self.client.delete('{_url}/users'.format(_url=self._url(id)), params={'email': email})

or use the same return self.client.delete('{}/users'.format(self._url(id)), params={'email': email}).

or can use return self.client.delete(self._url(id) + '/users', params={'email': email}) without formatting.

Let me know your thought on this.

Let's leave it as it was before:

self._url(id) + '/users'

I don't think the string formatting is adding anything here.

Ok, left it as it was before.

joshcanhelp

LGTM! Thanks again!

vkmrishad · 2018-11-07T18:52:33Z

Thank you :)

vkmrishad added 2 commits October 11, 2018 00:01

Updated string format

b2f831d

Updated character

120b505

joshcanhelp reviewed Oct 22, 2018

View reviewed changes

joshcanhelp requested a review from lbalmaceda October 26, 2018 19:15

joshcanhelp suggested changes Nov 5, 2018

View reviewed changes

vkmrishad added 2 commits November 6, 2018 01:07

String formatting updated with {}.format()

134ec27

Code fix

4c915e6

joshcanhelp suggested changes Nov 6, 2018

View reviewed changes

vkmrishad added 2 commits November 7, 2018 10:49

Code reverted

a1cc16b

Code reverted.

b45e112

joshcanhelp approved these changes Nov 7, 2018

View reviewed changes

lbalmaceda merged commit 777fb97 into auth0:master Nov 7, 2018

lbalmaceda added the CH: Changed label Nov 7, 2018

lbalmaceda added this to the v3-Next milestone Nov 7, 2018

lbalmaceda modified the milestones: v3-Next, 3.4.0 Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

String Formatting Updated #141

String Formatting Updated #141

vkmrishad commented Oct 10, 2018 •

edited

Loading

joshcanhelp left a comment

vkmrishad commented Oct 23, 2018

joshcanhelp commented Oct 23, 2018

vkmrishad commented Oct 29, 2018

joshcanhelp commented Oct 29, 2018

vkmrishad commented Oct 30, 2018

joshcanhelp commented Oct 30, 2018

vkmrishad commented Oct 31, 2018

joshcanhelp left a comment

joshcanhelp Nov 5, 2018

vkmrishad Nov 5, 2018 •

edited

Loading

vkmrishad Nov 5, 2018

vkmrishad commented Nov 6, 2018

joshcanhelp commented Nov 6, 2018

vkmrishad commented Nov 6, 2018

joshcanhelp left a comment

joshcanhelp Nov 6, 2018

vkmrishad Nov 7, 2018

joshcanhelp Nov 6, 2018

vkmrishad Nov 7, 2018 •

edited

Loading

joshcanhelp Nov 7, 2018

vkmrishad Nov 7, 2018

joshcanhelp left a comment

vkmrishad commented Nov 7, 2018

String Formatting Updated #141

String Formatting Updated #141

Conversation

vkmrishad commented Oct 10, 2018 • edited Loading

joshcanhelp left a comment

Choose a reason for hiding this comment

vkmrishad commented Oct 23, 2018

joshcanhelp commented Oct 23, 2018

vkmrishad commented Oct 29, 2018

joshcanhelp commented Oct 29, 2018

vkmrishad commented Oct 30, 2018

joshcanhelp commented Oct 30, 2018

vkmrishad commented Oct 31, 2018

joshcanhelp left a comment

Choose a reason for hiding this comment

joshcanhelp Nov 5, 2018

Choose a reason for hiding this comment

vkmrishad Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

vkmrishad Nov 5, 2018

Choose a reason for hiding this comment

vkmrishad commented Nov 6, 2018

joshcanhelp commented Nov 6, 2018

vkmrishad commented Nov 6, 2018

joshcanhelp left a comment

Choose a reason for hiding this comment

joshcanhelp Nov 6, 2018

Choose a reason for hiding this comment

vkmrishad Nov 7, 2018

Choose a reason for hiding this comment

joshcanhelp Nov 6, 2018

Choose a reason for hiding this comment

vkmrishad Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

joshcanhelp Nov 7, 2018

Choose a reason for hiding this comment

vkmrishad Nov 7, 2018

Choose a reason for hiding this comment

joshcanhelp left a comment

Choose a reason for hiding this comment

vkmrishad commented Nov 7, 2018

vkmrishad commented Oct 10, 2018 •

edited

Loading

vkmrishad Nov 5, 2018 •

edited

Loading

vkmrishad Nov 7, 2018 •

edited

Loading