Adding nginxcfg to useful scripts #20
Conversation
LukeShirnia
commented
Feb 25, 2020
LukeShirnia
commented
| except OSError: | ||
| return [] | ||
|
|
||
| def _get_all_config(self, config_file=None): |
There was a problem hiding this comment.
This might be handy as an argument for choosing a non-default config file (custom or source installation).
| } | ||
|
|
||
|
|
||
| class nginxCfg: |
There was a problem hiding this comment.
Comment: Possibly not needed as a Class -? Lot's of self function referencing, but no __init__ or variable definition.
nginxcfg/nginxcfg.py
Outdated
| config_data = open(config_file, 'r').readlines() | ||
|
|
||
| for line in [line.strip().strip(';') for line in config_data]: | ||
| if line.startswith('#'): | ||
| continue | ||
| line = line.split('#')[0] | ||
| if line.startswith('include'): |
There was a problem hiding this comment.
I'd generally recommend reading line by line vs. all into memory (not that the performance here requires it).
It is very doable here with no extra change - you are just looping over the readlines() return currently... instead just loop over the file object:
| config_data = open(config_file, 'r').readlines() | |
| for line in [line.strip().strip(';') for line in config_data]: | |
| if line.startswith('#'): | |
| continue | |
| line = line.split('#')[0] | |
| if line.startswith('include'): | |
| for line in [l.strip().rstrip(';') for l in open(config_file, 'r')]: | |
| line = line.split('#')[0] | |
| if line.startswith('include'): | |
| line = line.split('#')[0] |
(The startswith("#") is also not needed as you already check for "include")
In an ideal world, I'd suggest combining the looping to find includes with the config parsing vs. reading the files twice.
Verifies, though still pending comment responses |
subrepo: subdir: "nginxcfg" merged: "1e0ce24" upstream: origin: "https://github.com/LukeShirnia/nginxcfg.git" branch: "master" commit: "1e0ce24" git-subrepo: version: "0.4.1" origin: "https://github.com/ingydotnet/git-subrepo" commit: "a04d8c2"
LukeShirnia
force-pushed
the
nginxcfg-2020-02-25
branch
from
69e4f3c to
aa90cb7
Compare
| return server_dict_ret | ||
|
|
||
| def get_vhosts(self, exclude_string): | ||
| vhosts_list = self._get_vhosts() |
There was a problem hiding this comment.
Comment: All that happens with vhosts_list is you iterate through it. Would probably be preferred to iterating through the function directly.
Further to that, the function itself is a fairly simply loop. It's definitely beneficial to split the code out into functions - though the separation here seems less worthwhile.
ret = []
for f in self._get_all_config():
ret += self._get_vhosts_info(f)
return ret
I know there are larger plans to further rework bits (more than this minimal touch update), so no requirement for this now.
| open_brackets = 0 | ||
| found_server_block = False | ||
| for line_number, line in enumerate(vhost_data): | ||
| if line.startswith('#'): |
There was a problem hiding this comment.
Doesn't appear to be an issue, but this doesn't account for indented comments - it's either not needed because maybe you deal with comments later on, or it's worth adding a line.lstrip() here? 🤷♂
#TESTING Not picked up
#TESTING correctly skipped
|
