Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log to stderr by default instead of salt-server.log #213

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Log to stderr by default instead of salt-server.log #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a258323
Change the default --log-level to warning
koppchen Sep 4, 2022
336590f
Log to stderr by default instead of salt-server.log
koppchen Sep 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions salt_lsp/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@ def add_arguments(parser):
help="initialize the server, but don't launch it "
"(useful for debugging/testing purposes)",
)
parser.add_argument(
"--log-file",
help="Redirect logs to the given file instead of writing to stderr",
)
Comment on lines +46 to +49
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parser.add_argument(
"--log-file",
help="Redirect logs to the given file instead of writing to stderr",
)
parser.add_argument(
"--log-file",
help="Redirect logs to the given file instead of writing to stderr",
nargs=1,
type=str,
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type: Hmm checking the docs for type, it looks like it defaults to string, which agrees with what I see while testing. So we should be ok there, however we can add it to be more explicit if we want?

From https://docs.python.org/3/library/argparse.html#type

By default, the parser reads command-line arguments in as simple strings. However, quite often the command-line string should instead be interpreted as another type, such as a float or int.

nargs: Ahh, I recall having trouble with nargs in the past since it can be a little confusing. Looks like using nargs=1 would convert it to a list, which would require the [0] usage later on.

From https://docs.python.org/3/library/argparse.html#nargs

The supported values are:

  • N (an integer). N arguments from the command line will be gathered together into a list.
    ...
    Note that nargs=1 produces a list of one item. This is different from the default, in which the item is produced by itself.

It might be more clear to leave the nargs=1 off, so we don't need the [0] later?

parser.add_argument(
"--log-level",
choices=list(LOG_LEVEL_DICT.keys())
+ list(map(lambda level: level.upper(), LOG_LEVEL_DICT.keys())),
default=["debug"],
default=["warning"],
nargs=1,
help="Logging verbosity",
)
Expand All @@ -60,7 +64,7 @@ def main():

log_level = loglevel_from_str(args.log_level[0])
logging.basicConfig(
filename="salt-server.log",
filename=args.log_file,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be?

Suggested change
filename=args.log_file,
filename=args.log_file[0],

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [0] would be needed if nargs=1 is used. (as mentioned in the other comment)

Oh, btw, I am doing something with this that might be a little confusing. If --log-file isn't passed in as an arg, then args.log_file defaults to None, and that's still passed to basicConfig(). However, basicConfig sets filename to None by default if it's not specified, which is why this works fine.

The related basicConfig() code is here:
https://github.com/python/cpython/blob/main/Lib/logging/__init__.py#L2118-L2129

level=log_level,
filemode="w",
)
Expand All @@ -72,7 +76,7 @@ def main():

salt_server = SaltServer()
setup_salt_server_capabilities(salt_server)
salt_server.post_init(states, log_level)
salt_server.post_init(states)

if args.stop_after_init:
return
Expand Down
2 changes: 0 additions & 2 deletions salt_lsp/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,12 @@ def workspace(self) -> SlsFileWorkspace:
def post_init(
self,
state_name_completions: Dict[str, StateNameCompletion],
log_level=logging.DEBUG,
) -> None:
"""Further initialisation, called after
setup_salt_server_capabilities."""
self._state_name_completions = state_name_completions
self._state_names = list(state_name_completions.keys())
self.logger = logging.getLogger(self.__class__.__name__)
self.logger.setLevel(log_level)

def complete_state_name(
self, params: types.CompletionParams
Expand Down
4 changes: 1 addition & 3 deletions salt_lsp/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
contents utilizing the existing Workspace implementation from pygls.

"""
from logging import getLogger, Logger, DEBUG
from logging import getLogger, Logger
from pathlib import Path
import sys
from typing import List, Optional, Union
Expand Down Expand Up @@ -59,8 +59,6 @@ def __init__(
self._state_name_completions = state_name_completions

self.logger: Logger = getLogger(self.__class__.__name__)
# FIXME: make this configurable
self.logger.setLevel(DEBUG)

super().__init__(*args, **kwargs)

Expand Down