131 locate free extensions #139
Conversation
Description Available In Docstring
odins_spear/scripter.py
Outdated
| @@ -167,6 +167,9 @@ def remove_numbers(self, service_provider_id: str, group_id: str, start_of_range | |||
| return scripts.remove_numbers.main(self.api, service_provider_id, group_id, start_of_range_number, | |||
| end_of_range_number) | |||
|
|
|||
| def locate_free_extension(self, service_provider_id: str, group_id: str, field: list): | |||
|
|
|||
| return scripts.locate_free_extension(self.api, service_provider_id, group_id, field) | |||
There was a problem hiding this comment.
Have you ran the code? Missing '.main()' here on the end of 'locate_free_extension' which would have errored if you ran it.
| group_id | ||
| ) | ||
|
|
||
| for data in dataset: |
There was a problem hiding this comment.
This is a little repetitive looping three times, can we be more efficient here collecting all data and looping once maybe?
| if not extensions: | ||
| return | ||
|
|
||
| return extensions |
There was a problem hiding this comment.
Condensed version of lines 58-61:
return extensions if extensions else None
| ### | ||
|
|
||
| ### Locate Initial Free Extension | ||
| extension_set = set(extensions) |
There was a problem hiding this comment.
Not sure why you've done this dude, extensions is already an iterable. Converting to a set is usually only done when we want to remove duplicates or need specific set functionality.
| ### Locate Initial Free Extension | ||
| extension_set = set(extensions) | ||
|
|
||
| for extension in range(field[0] + 1, field[1]): |
There was a problem hiding this comment.
Why do you skip the first extension in the range by adding 1? Range starts at the first value and goes up to but doesn't include the last value so would have thought you'd add 1 to the end to include the last value.
There was a problem hiding this comment.
I believed the range would be better suitable for extensions between the passed range values however
| extension_set = set(extensions) | ||
|
|
||
| for extension in range(field[0] + 1, field[1]): | ||
| if extension not in extension_set: |
There was a problem hiding this comment.
Does this work as extensions will be a set of strings right like '100' and you are looking for an integer just 100.
|
|
||
| for extension in range(field[0] + 1, field[1]): | ||
| if extension not in extension_set: | ||
| return extension |
There was a problem hiding this comment.
This is just keeping convention we have but here return a python dict like the below.
return {'extension': extension}
| return extension | ||
| #### | ||
|
|
||
| print("Unable To Locate A Free Extension Within The Range") |
There was a problem hiding this comment.
Here we want to raise an a new error that the user hasn't been found. The reason we do this is if a developer has a script that uses this function to first find a free extension and in the next step takes what is returned from this and inserts into a new function to build it printing we didnt find it has no control flow value. We need to interrupt otherwise we will cause rurther errors down the line.
As per Jordan's comment review, I have improved the script to better suite the codebase
| '''Retrieves The Lowest Free Extension Available In The Designated Group Passed. | ||
| ''' | ||
|
|
||
| if extension_range[0] > extension_range[1]: |
There was a problem hiding this comment.
I think we can avoid this range adjusting by separating into two params and raise an error if the end is larger. Instead of extension_range remove this and introduce 'start_of_range' and 'end_of_range'. Then in the code you can create a new error and do the below:
if start_of_range > end_of_range:
raise OSInvalidInput <- example (check if we have an existing error you could throw)
| group_id, | ||
| ) | ||
|
|
||
| for extension in range(extension_range[0], extension_range[1] + 1): |
There was a problem hiding this comment.
You can update this inline with the new params
| api, | ||
| service_provider_id: str, | ||
| group_id: str, | ||
| extension_range: list |
There was a problem hiding this comment.
Still the old param not the new start of and end of range.
No description provided.