New programming model lib change#99
Conversation
Minor; flake8 issues. Updated Decorator logic and placing Aliasing function name Adding route
…plication, separate decorator to one per binding/property for decoupling, some refactors
…t route to function name; add route decorator tests
…efault to function_app.py
…n string, change enums to return value, change tests
YunchuWang
requested review from
anirudhgarg,
gavin-aguiar and
vrdmr
as code owners
✅ Result of Pytest Coverage---------- coverage: platform linux, python 3.6.15-final-0 -----------
|
…ict repr documentid to id
vrdmr
left a comment
vrdmr
left a comment
There was a problem hiding this comment.
Round 2 comments. Will complete the review by tonight.
| from ._cosmosdb import Document, DocumentList | ||
| from ._http import HttpRequest | ||
| from ._http import HttpResponse | ||
| from .decorators import (FunctionApp, Function, DataType, AuthLevel, |
There was a problem hiding this comment.
Please keep the imports in the way they were already - The ._* imports separate from .* imports - for now. We should consolidate and the imports better soon - and explore isort in future during tests - or recommit hooks (or integrating it with PyCharm).
| return words[0] + ''.join(ele.title() for ele in words[1:]) | ||
|
|
||
|
|
||
| class CustomJsonEncoder(JSONEncoder): |
There was a problem hiding this comment.
Just a one-line comment - of why you needed a CustomJSONEncoder. Also, the update class name to CustomJSONEncoder to denote the subclass of the JSONEncoder.
There was a problem hiding this comment.
Can you think of a better name than Custom - something which signifies what is custom about this JSONEncoder?
There was a problem hiding this comment.
Just a one-line comment - of why you needed a CustomJSONEncoder. Also, the update class name to
CustomJSONEncoderto denote the subclass of the JSONEncoder.
Making enum str or int as first subclass i tried will make it a str or int type instead of enum, and
def parse_singular_param_to_enum(param: Optional[Union[T, str]], class_name: Type[T]) -> Optional[T]: would fail as no way to tell whether its a "GET" or HttpMethod.GET, they have same type str.
|
|
||
|
|
||
| class TestCosmosDB(unittest.TestCase): | ||
| def test_cosmos_db_trigger_valid_creation(self): |
There was a problem hiding this comment.
Also add a test scenario where you are sending only the minimal stuff - we want to check the function.json when Nones are automatically set
There was a problem hiding this comment.
This comment applies to all the triggers and bindings.
There was a problem hiding this comment.
Need not be part of this PR though
There was a problem hiding this comment.
Okay - I got my partial answer - you are validating function.json with fewer args.
| self.assertTrue(isinstance(func.get_trigger(), HttpTrigger)) | ||
| self.assertTrue(func.get_trigger().route, "dummy") | ||
|
|
||
| def test_timer_trigger_default_args(self): |
There was a problem hiding this comment.
Something to think about - as this test file is becoming bigger - have you explored some thoughts to breakup the tests into its own classes
A top-level class TestDecorators can have the basic stuff that individual test classes can inherit (eg the self.func_app = FunctionApp() as common setup)
and then class TestHTTPDecorators(TestDecorators), class TestTimerDecorators(TestDecorators), etc. can come in to make the handling of this long file easier. And also helps in test management on our end.
There was a problem hiding this comment.
Not for this PR though. CodeCoverage is more important currently.
4 participants
New programming model library change