Conversation
|
|
|
||
| user_config = load_config_file(config_path) | ||
| self.update(**user_config) | ||
| if config_path: |
There was a problem hiding this comment.
Hey, can we have some tests to this part?
There was a problem hiding this comment.
Here we have some guidelines for testing with you want to take a look at it: https://github.com/scanapi/scanapi/blob/master/CONTRIBUTING.md#tests
Let me know if you need any help!
|
| return json.load(f) | ||
| else: | ||
| raise FileFormatNotSupportedError(f".{extension}", relative_path) | ||
| return yaml.load(f, Loader) |
There was a problem hiding this comment.
I know is just 2 lines of code, but can we use a more meaningful name rather than just a single letter? :/
How about
| return yaml.load(f, Loader) | |
| return yaml.load(file_hander, Loader) |
instead? Don't forget to update line 37 if you accept this.
There was a problem hiding this comment.
If anything I'd pick file or handle which are both better than f, which one do you prefer?
There was a problem hiding this comment.
Maybe handle, as I'm not sure right now if file is a reserved name/module in python. That's why suggested file_handler.
There was a problem hiding this comment.
file is the name of a builtin in Python 2, but not in Python 3 :)
There was a problem hiding this comment.
Did not know that! Unfortunately I'm still stuck with some Python 2 projects :(
Guess we can go with file then
| except yaml.YAMLError as e: | ||
| logger.error(e) |
There was a problem hiding this comment.
Maybe
| except yaml.YAMLError as e: | |
| logger.error(e) | |
| except yaml.YAMLError as excep: | |
| logger.error(excep) |
instead?
There was a problem hiding this comment.
e is a pretty common name so this one I'm unsure about.
| if not any( | ||
| [config_path, self.has_local_config_file, self.has_global_config_file] | ||
| ): | ||
| return |
There was a problem hiding this comment.
Do we really need these check? The next couple of lines will perform if-elifs, and if all options are false, then it will already (implicitly) return None, like what is being done over here.
| if not any( | |
| [config_path, self.has_local_config_file, self.has_global_config_file] | |
| ): | |
| return |
There was a problem hiding this comment.
Yea I agree I'll change that.
vinigfer
changed the title
|
@supakeen hey, do you intend to keep working on this PR? Or do you want me to get it from here? |
|
@camilamaia If I don't do it this weekend you can take it but my plan is to get it done then :) |
|
Hey, I've changed the review requests but I'm now slightly confused while rebasing. It seems master has a new |
|
There we go, I think I got the rebase working. @camilamaia I don't have time in the foreseeable future due to holiday season here to add testcases as well perhaps someone could take over this PR for that? :) |
|
@supakeen great, no problem! I will take it, thanks for the effort! |
|
Closed by #254 |
3 participants
Initial support for a different configuration filename and global configuration.
closes #203