-
Notifications
You must be signed in to change notification settings - Fork 577
Migrated commit_represent to S3Represent #1182
base: master
Are you sure you want to change the base?
Conversation
modules/s3db/req.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes no difference with S3Represent if you call other represents per-row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should call the bulk-method of the same (!) represent instances once during lookup_rows for /all/ values.
That will store the representations in the instance, so that when you call them per-row in represent_row, they wouldn't be looked up again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor does basically nothing - so should be removed.
This is the right concept - but a sloppy implementation of it ;) You should always test your code before submitting a PR - and this code can not work, so you either didn't test it, or you ignored that it doesn't work ;) |
Actually @nursix I did tried to test it, but I think they way I tested it is wrong, sorry for that. |
@anurag-ks - generally no problem to ask for feedback on the concept by tagging me in a comment to your commit before submitting a PR. But for a PR, stuff should be working. You could try to write a unit test for this (even if just as an exercise) - this would give you immediate feedback while refactoring things. |
@nursix just a doubt, these represent classes/functions are used for custom rendering of data values for data-tables or exporting data, right ? But on enabling the |
for req/commit there is no 'commit_id' field, there is an 'id' field which in other places would be seen as a commit_id |
yes |
Okay so |
A unit test would create a test req_commit record, and then run req_CommitRepresent against the record ID. Then check whether the return value is correct, and finally remove the test record. That's all there is to it, nothing fancy. |
There are over 500 examples of unit tests in modules/unit_tests and plenty of documentation for the Python unittest module. It's not rocket science - the trick is just to understand what you're trying to prove with the test and implement a valid test. |
And I said that with the unittest only to spare you the effort of digging for a case where it is visible, because I understood that you couldn't find one. In fact, there may not be one ;) |
No description provided.