[Controller] Introduces Rules For Kwargs Checking #57
Conversation
…s requirements and be a single point of failure given wrong kwargs are passed
facebook-github-bot
added
the
CLA Signed
|
@OmarMuhammedAli I implemented the |
|
@Rubix982 Yep! let's do that |
| if ("min_date" in kwargs or "max_date" in kwargs) and ("daterange" in kwargs): | ||
|
|
||
| # Check if two contradicting keys have not been given | ||
| raise ContradictingError( | ||
| contradicts="daterange", | ||
| contradicted="min_date/max_date", | ||
| message="Using either or both of min_date and max_date, or use daterange, " | ||
| "but not both at the same time", | ||
| ) |
There was a problem hiding this comment.
Can you explain what this snippet does? what constitutes contradiction between different kwargs?
There was a problem hiding this comment.
In the case of the example here, if the user decides to give the date in the format of a list for the key date range, and then also pass in max_date, and min_date, it becomes confusing about what the user actually wants to filter through - the provided list or the individually given arguments.
In the case where we remove daterange altogether, this wouldn't need to be here.
Given that context, this code snippet throws a ContradictingError when both the kwargs are provided.
@cbeddow should we remove daterange all together, and just keep a min_date and a max_date argument?
There was a problem hiding this comment.
I think having either min_date and max_date or daterange as a chronological filtering mechanism will be sufficient, having both is redundant imo.
There was a problem hiding this comment.
Agreed. If a user can turn the dates into a list to pass to date range, they can just pass to max_date, and min_date as well, but I'll wait over what @cbeddow has to say about it.
There was a problem hiding this comment.
Yes, remove daterange, this seems to be the same end, a range from min_date to max_date
Thanks! For the "what they are for each check, what are they based on" suggestion, what should I add? Will the explanation in the DocStrings of |
Committing suggestions as per review Co-authored-by: Omar Ali <omarmuhammed1998.om@gmail.com>
Oh I've just seen the explanations in |
|
@OmarMuhammedAli okieeesss. Should I self-merge, or you? |
|
Just merged! |
|
Ly! |
Introduced "rules" for controller/, helps to implement DRY for various requirements and be a single point of failure given wrong
keyword argumentsare passed.Compliments #56