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

🚀 Feature: use fstring instead str.replace() ? #48

Open
2 tasks done
dan-gut1 opened this issue Oct 6, 2022 · 11 comments
Open
2 tasks done

🚀 Feature: use fstring instead str.replace() ? #48

dan-gut1 opened this issue Oct 6, 2022 · 11 comments
Assignees

Comments

@dan-gut1
Copy link

dan-gut1 commented Oct 6, 2022

🔖 Feature description

Why does the SDK uses str.replace() instead fstring?.

🎤 Pitch

In the past few days I'm working on implementing the front of appwrite in python for learning purposes and I've note that the 'python server sdk' mostly uses str.replace() for its calls. increasing processing time on each call of str.replace().

So it raise the question why these calls doesn't uses fstring?, it will greatly reduce processing time.

for example from Services.Databases:

 def update_document(self, database_id, collection_id, document_id, data = None, permissions = None):
        """Update Document"""

        
        path = '/databases/{databaseId}/collections/{collectionId}/documents/{documentId}'
        params = {}
        if database_id is None:
            raise AppwriteException('Missing required parameter: "database_id"')

        if collection_id is None:
            raise AppwriteException('Missing required parameter: "collection_id"')

        if document_id is None:
            raise AppwriteException('Missing required parameter: "document_id"')

        path = path.replace('{databaseId}', database_id)
        path = path.replace('{collectionId}', collection_id)
        path = path.replace('{documentId}', document_id)

        params['data'] = data
        params['permissions'] = permissions

        return self.client.call('patch', path, {
            'content-type': 'application/json',
        }, params)

Instead it is possible to use fstring, but need to note that there is lack of compliance with python under version 3.6.

 def update_document(self, database_id, collection_id, document_id, data = None, permissions = None):
        """Update Document"""

        
        params = {}
        if database_id is None:
            raise AppwriteException('Missing required parameter: "database_id"')

        if collection_id is None:
            raise AppwriteException('Missing required parameter: "collection_id"')

        if document_id is None:
            raise AppwriteException('Missing required parameter: "document_id"')
        
       # bare minimum cpu usage
        path = f'/databases/{databaseId}/collections/{collectionId}/documents/{documentId}'

        params['data'] = data
        params['permissions'] = permissions

        return self.client.call('patch', path, {
            'content-type': 'application/json',
        }, params)

👀 Have you spent some time to check if this issue has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

@dan-gut1 dan-gut1 changed the title 🚀 Feature: use str.replace() instead fstring? 🚀 Feature: use fstring instead str.replace() ? Oct 6, 2022
@stnguyen90
Copy link
Contributor

lack of compliance with python under version 3.6.

As you suggested, f strings are not as compatible so it might be better if we use str.format() instead of str.replace().

@dan-gut1
Copy link
Author

dan-gut1 commented Oct 6, 2022

So if so, I guess it well be better for write it with future f-string support as follow ?

'/databases/{databaseId}/collections/{collectionId}/documents/{documentId}'.format(databaseId=databaseId, collectionId=collectionId, documentId=documentId )

So in future time it will be easily change to native fstring?

As far as I saw fstring faster then string formating.

@GbotemiB
Copy link

GbotemiB commented Oct 8, 2022

Hi, I will like to work on this task.

@rahulsunil2
Copy link

I would like to work on this.

@stnguyen90
Copy link
Contributor

@dan-gut1, let's go with str.format() approach. Would you like to work on this?

@dan-gut1
Copy link
Author

dan-gut1 commented Oct 13, 2022

Would you like to work on this?

I will be delighted, although there is enough work for all of us, if you would like to split the work among us no issue with that.

Can you please reference us to the, correct "HOW" to start contribution to code\python SDK repo?, and tests.

@stnguyen90
Copy link
Contributor

@dan-gut1, best for only one person to work on this. Please refer to:

  1. Template for service: https://github.com/appwrite/sdk-generator/blob/2637d47c7d1e5058a6f572078437f432bd082e71/templates/python/package/services/service.py.twig#L16
  2. Instructions on how to contribute: https://github.com/appwrite/sdk-generator/blob/master/CONTRIBUTING.md
  3. Script to generate SDKs: https://github.com/appwrite/sdk-generator/blob/master/example.php

Please reach out if you're stuck with anything.

@dan-gut1
Copy link
Author

dan-gut1 commented Oct 17, 2022

So in the middle way of making the changes, I was asked myself if these changes are worth doing, I made a little test, and its result not so good (at least in me opinion).

The test I made as follow with 3 variables.

  1. Test the execution speed the current creation of the path using str.replace().
  2. Test the execution speed the current creation of the path using str.format with variable assignment.
  3. Test the execution speed the current creation of the path using str.format by variable indexing.
  4. Test the execution speed the current creation of the path using fstring.

Here are brief result using timeit.timeit

replace path: 0.5664167
format with assignment: 0.9150742999999999
format with indexs: 0.5907963000000003
fstring path: 0.3794525000000002

you can find the test code here - https://pastebin.com/FtY1XXcW

So it raise a question.

Is the changing I'm working on worth doing?,
although it make the path creation much more standard, I use format with assignment which is the worst time consuming. so I think that if we're not using fstring there's no point to continue with the modifications.

@stnguyen90 let me know what you thinking.

@stnguyen90
Copy link
Contributor

@dan-gut1 heh...probably not since it's already working 😅

@ayaan-qadri
Copy link

ayaan-qadri commented Jun 14, 2024

Since f-string can be implement for version 3.6 or above instead we can use string concatenation as well, it support all python version and also more faster than all (still not faster than f-string)

Edit: Using ''.join we can achieve even more faster than concatenation (We can use it python version 1.6 or above)

replace path: 0.49085860000923276
format with assignment: 0.7151281999831554
format with index: 0.3822980000113603
fstring path: 0.15623890000279061
concatenation path: 0.24857970001176
join path: 0.19559379998827353

Here is the added string concatenation and join method performance check code in the dan-gut1's code: https://pastebin.com/XRPJpHRi

@ayaan-qadri
Copy link

@stnguyen90 pinging you, if you missed and want to know your opinion.

Since f-string can be implement for version 3.6 or above instead we can use string concatenation as well, it support all python version and also more faster than all (still not faster than f-string)

Edit: Using ''.join we can achieve even more faster than concatenation (We can use it python version 1.6 or above)

replace path: 0.49085860000923276 format with assignment: 0.7151281999831554 format with index: 0.3822980000113603 fstring path: 0.15623890000279061 concatenation path: 0.24857970001176 join path: 0.19559379998827353

Here is the added string concatenation and join method performance check code in the dan-gut1's code: https://pastebin.com/XRPJpHRi

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

No branches or pull requests

5 participants