Skip to main content
added 1 character in body
Source Link
toolic
  • 15.6k
  • 5
  • 29
  • 216

argparse

When I run the code without passing a command-line argument, it dies badly with error messages. It would be better if it died with a helpful message which clearly shows the intended usage.

Also, when just using the --help option, it shows all arguments as being optional. However, as mentioned above, the code does require the user to specify something on the command line. argparse can be configured to indicate required arguments.

Option names are typically short, especially in a case like this where there are just 3 options. It would be more convenient for the user to rename paths_configuration_path as something like paths.

Simpler

This code creates several unnecessary variables:

paths_configuration_path=args.paths_configuration_path
key=args.key

paths = get_paths_from_configuration(paths_configuration_path=paths_configuration_path, key=key)

return paths

It can be simplified as:

return get_paths_from_configuration(
    paths_configuration_path = args.paths_configuration_path,
    key = args.key
)

Whitespace

The code uses inconsistent whitespace around the = operators; some have space, while otherothers do not. The black program can be used to automatically format your code.

Naming

The function named get_paths_config_from_cl_arguments seems unnecessarily long. I think get_paths_config is sufficient. You can add a docstring to summarize its purpose:

def get_paths_config(default_config_path: Path | None, default_key:str=None)->dict:
    """ Use command-line arguments to read configuration file. """

Test

It is great that you provided a sample JSON input file in the question, but I get errors on these Python code lines when I run the code:

data_path = paths['dataset_path']
variables_metadata_path = paths['variables_description_path']

argparse

When I run the code without passing a command-line argument, it dies badly with error messages. It would be better if it died with a helpful message which clearly shows the intended usage.

Also, when just using the --help option, it shows all arguments as being optional. However, as mentioned above, the code does require the user to specify something on the command line. argparse can be configured to indicate required arguments.

Option names are typically short, especially in a case like this where there are just 3 options. It would be more convenient for the user to rename paths_configuration_path as something like paths.

Simpler

This code creates several unnecessary variables:

paths_configuration_path=args.paths_configuration_path
key=args.key

paths = get_paths_from_configuration(paths_configuration_path=paths_configuration_path, key=key)

return paths

It can be simplified as:

return get_paths_from_configuration(
    paths_configuration_path = args.paths_configuration_path,
    key = args.key
)

Whitespace

The code uses inconsistent whitespace around the = operators; some have space, while other do not. The black program can be used to automatically format your code.

Naming

The function named get_paths_config_from_cl_arguments seems unnecessarily long. I think get_paths_config is sufficient. You can add a docstring to summarize its purpose:

def get_paths_config(default_config_path: Path | None, default_key:str=None)->dict:
    """ Use command-line arguments to read configuration file. """

Test

It is great that you provided a sample JSON input file in the question, but I get errors on these Python code lines when I run the code:

data_path = paths['dataset_path']
variables_metadata_path = paths['variables_description_path']

argparse

When I run the code without passing a command-line argument, it dies badly with error messages. It would be better if it died with a helpful message which clearly shows the intended usage.

Also, when just using the --help option, it shows all arguments as being optional. However, as mentioned above, the code does require the user to specify something on the command line. argparse can be configured to indicate required arguments.

Option names are typically short, especially in a case like this where there are just 3 options. It would be more convenient for the user to rename paths_configuration_path as something like paths.

Simpler

This code creates several unnecessary variables:

paths_configuration_path=args.paths_configuration_path
key=args.key

paths = get_paths_from_configuration(paths_configuration_path=paths_configuration_path, key=key)

return paths

It can be simplified as:

return get_paths_from_configuration(
    paths_configuration_path = args.paths_configuration_path,
    key = args.key
)

Whitespace

The code uses inconsistent whitespace around the = operators; some have space, while others do not. The black program can be used to automatically format your code.

Naming

The function named get_paths_config_from_cl_arguments seems unnecessarily long. I think get_paths_config is sufficient. You can add a docstring to summarize its purpose:

def get_paths_config(default_config_path: Path | None, default_key:str=None)->dict:
    """ Use command-line arguments to read configuration file. """

Test

It is great that you provided a sample JSON input file in the question, but I get errors on these Python code lines when I run the code:

data_path = paths['dataset_path']
variables_metadata_path = paths['variables_description_path']
Source Link
toolic
  • 15.6k
  • 5
  • 29
  • 216

argparse

When I run the code without passing a command-line argument, it dies badly with error messages. It would be better if it died with a helpful message which clearly shows the intended usage.

Also, when just using the --help option, it shows all arguments as being optional. However, as mentioned above, the code does require the user to specify something on the command line. argparse can be configured to indicate required arguments.

Option names are typically short, especially in a case like this where there are just 3 options. It would be more convenient for the user to rename paths_configuration_path as something like paths.

Simpler

This code creates several unnecessary variables:

paths_configuration_path=args.paths_configuration_path
key=args.key

paths = get_paths_from_configuration(paths_configuration_path=paths_configuration_path, key=key)

return paths

It can be simplified as:

return get_paths_from_configuration(
    paths_configuration_path = args.paths_configuration_path,
    key = args.key
)

Whitespace

The code uses inconsistent whitespace around the = operators; some have space, while other do not. The black program can be used to automatically format your code.

Naming

The function named get_paths_config_from_cl_arguments seems unnecessarily long. I think get_paths_config is sufficient. You can add a docstring to summarize its purpose:

def get_paths_config(default_config_path: Path | None, default_key:str=None)->dict:
    """ Use command-line arguments to read configuration file. """

Test

It is great that you provided a sample JSON input file in the question, but I get errors on these Python code lines when I run the code:

data_path = paths['dataset_path']
variables_metadata_path = paths['variables_description_path']