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.

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

Code formatting from CFGrote #22

Open
shervin86 opened this issue Oct 4, 2021 · 3 comments
Open

Code formatting from CFGrote #22

shervin86 opened this issue Oct 4, 2021 · 3 comments

Comments

@shervin86
Copy link
Contributor

Very good work, I only have some cosmetic comments:

  • Please check pep8 (code formatting). e.g empty line at the bottom.

  • init(): I would be stricter on having all attributes hidden away as self.__attribute and define accessors for getting and setting attribute values, decorated as @property and setter, respectively.

  • Docstrings: I always leave a blank line between two :type ...: and the next :param...: and don't mix google, numpy, and doxygen styles.

Originally posted by @CFGrote in #19 (review)

@JunCEEE
Copy link
Contributor

JunCEEE commented Oct 15, 2021

@CFGrote
Done. See it here:

"""
:module Instrument: Module hosting the Instrument class
"""
from libpyvinyl.Parameters.Collections import InstrumentParameters
from pathlib import Path
class Instrument():
"""An Instrument class"""
def __init__(self, name, calculators=None):
"""An Instrument class
:param name: The name of this instrument
:type name: str
:param calculators: A collection of Calculator objects.
:type calculators: dict
"""
self.__name = name
self.__calculators = {}
self.__parameters = InstrumentParameters()
if calculators is not None:
for calculator in calculators:
self.add_calculator(calculator)
def add_master_parameter(self, name, links, **kwargs):
self.parameters.add_master_parameter(name, links, **kwargs)
@property
def name(self):
"""The name of this instrument."""
return self.__name
@name.setter
def name(self, value: str):
self.__name = value
@property
def calculators(self):
"""The list of calculators. It's modified either when constructing the class instance
or using the `add_calculator` function.
"""
return self.__calculators
@property
def parameters(self):
"""The parameter collection of each calculator in the instrument. These parameters are links to the
exact parameters of each calculator."""
return self.__parameters
@property
def master(self):
return self.parameters.master
def set_base_path(self, base: str):
"""Set each calculator's output_path as 'base_path/calculator.name'.
:param base: The base path to be set.
:type base: str
"""
self.base_path = base
basePath = Path(base)
for key in self.calculators:
outputPath = basePath / self.calculators[key].name
calculator = self.calculators[key]
calculator.output_path = str(outputPath)
def list_calculators(self):
string = f"- Instrument: {self.name} -\n"
string += "Calculators:\n"
for key in self.calculators:
string += f"{key}\n"
print(string)
def list_parameters(self):
print(self.parameters)
def add_calculator(self, calculator):
self.__calculators[calculator.name] = calculator
self.__parameters.add(calculator.name, calculator.parameters)
def remove_calculator(self, calculator_name):
del self.__calculators[calculator_name]
del self.__parameters[calculator_name]

@CFGrote
Copy link
Contributor

CFGrote commented Dec 1, 2021

So here are some coding guidelines:

Formatting

All variable names and functions should be snake_case, not camelCase.

  • OK:
def set_parameters(name, value): 
    my_variable = value
  • not ok:
def setParameters(name, value):
    myVariable = 10
  • Classes should be named in CamelCase style:
class MyClass():
...

Verbose naming

  • OK: def set_parameters()
  • not ok: def set_params()

Class definitions

class MyClass(object):
    """ :class MyClass: Encapsulates simulation of photon matter interaction """

    def __init__(self, par1, par2):
        """
        MyClass constructor.
        
        :param par1: <Docstring for par1>
        :type  par1: str

        :param par2: <Docstring for par2>
        :type  par2: int

        """
        # Init (declare) hidden attributes
        self.__par1 = self.__par2 = None

        # Assign construction parameters via setters. Setters check type and valid range of values.
        self.par1 = par1
        self.par2 = par2

        # Run consistency checks across parameters.
        self._check_consistency()
    
    @property
    def par1(self):
        """ Getter for the par1 attribute """
        return self.__par1
    
    @par1.setter
    def par1(self, val):
        """ Set the value of par1 """
        
        # Handle default value
        if val is None:
            val = "foo"

        # Run type checks
        if not isinstance(val, str):
            raise TypeError("Parameter `par1` must be of type str, received {}.".format(type(val))
        
        # Check permitted values
        if not val in ["list", "of", "permitted", "values"]:
            raise ValueError("Parameter `par1` must be one {}, not {}.".format(["list", "of", "permitted", "values"], val)
       
        self.__par1 = par1

You get the idea...

Docstrings

You see from the above that I prefer doxygen style docstrings but I'm not too religious about this. Should be consistent at least within one py file.

In most other code formatting questions, I follow the pip8 recommendations.

@shervin86
Copy link
Contributor Author

@CFGrote I think it would be useful to add to the documentation in the DEVEL.md file these guidelines.
We have used blank in order to get uniform formatting.
I would then close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants