1008028 - docstrings for all of mozharness.base.*

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Comment 1

[kartik gupta]

heyy Armen , i am interested in working on this.
Can you please elaborate a little bit more what exactly we have to do?

Comment 2

[Armen [:armenzg]]

(In reply to kartik gupta from comment #1)
> heyy Armen , i am interested in working on this.
> Can you please elaborate a little bit more what exactly we have to do?

Hi kartik,
From what aki wrote I assume you will need to do the following:
1 - Check out http://hg.mozilla.org/build/mozharness
2 - Change directories to mozharness/mozharness/base
3 - Look at which modules, classes, functions don't have docstrings and add/modify them

Docstrings are very useful for many things.
Some of them are:
1 - General Documentation
2 - Accessible when debugging
3 - Generate pretty documentation

If you could follow adding information in the same style as [1] that would be great.

In [2] I show how when there is docstrings we can do something like foo.__doc__

The topic of writing docstrings can be extensive. We can do a basic pass and see where we are after that. For the record, we tend to follow sphinx-compatible docstrings (i.e. ReST) so we can easily use them with readthedocs if we wish to [3]

Let me know if I can guide you in any way or go over any basics.

regards,
Armen

[1]
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#1366

[2]
armenzg@armenzg-thinkpad:~/repos/mozharness/mozharness/base$ python 
Python 2.7.6 (default, Mar 22 2014, 22:59:56) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import log
>>> dir(log)
['BaseLogger', 'CRITICAL', 'DEBUG', 'ERROR', 'FATAL', 'FATAL_LEVEL', 'IGNORE', 'INFO', 'LogMixin', 'MultiFileLogger', 'OutputParser', 'SimpleFileLogger', 'WARNING', '__builtins__', '__doc__', '__file__', '__name__', '__package__', 'datetime', 'logging', 'os', 'sys', 'traceback']
>>> log.__doc__
'Generic logging, the way I remember it from scripts gone by.\n\nTODO:\n- network logging support.\n- log rotation config\n'

[3]
http://legacy.python.org/dev/peps/pep-0287/#docstring-significant-features

Comment 3

[Camilo]

Hey Armen
I saw that this bug has no activity since 11.2014, is it still open? 
If so, I would like to contribute. I already created a patch for the mozharness before so I know a little bit how it works.

Regarding the documentation, how should we do it? Shall we do it in the same way that you say at the end of comment #2?

Cheers

Comment 4

[Armen [:armenzg]]

Hi carocad,
Sure!

Please let me know if you need any pointers.

Comment 5

[Simarpreet Singh]

Hi carocad,

Let me know if you need help on getting started, I'm usually lurking on the #ateam and #releng IRC channels.

Comment 6

[Camilo]

Attachment: LogMixing_docstring.patch

Hey Armen
I have added some docstrings to the LogMixin class. Please check them and let me know if they are ok so that I know if I am consistently making any errors.

Cheers

Comment 7

[Armen [:armenzg]]

Comment on attachment 8600711 [details] [diff] [review]
LogMixing_docstring.patch

Review of attachment 8600711 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Camilo for the patch! You seem on the right track. See if Aki made any comments at the beginning of the bug to see if anything else is required.
My apologies for the delay. I was out for half the day.

If the indent needs fixing, could you please attach a new patch and ask review from Simarpreet? (The flag show up on the UI for an attachment)
He's helping us by reviewing mozharness patches.

::: mozharness/base/log.py
@@ +48,4 @@
>      """
>  
>      def _log_level_at_least(self, level):
> +		""" Check if the current logging level is greater or equal than level

What is the reason that this and other comments are so far indented to the right? Is to meet any standards?

Comment 8

[Camilo]

Hey Armen
Good that the general structure is ok :)
As for the indenting, I just checked and it is because of the configuration of tab/spaces. I use a 4 spaces per tab configuration but it seems that firefox interprets my tabs as 8 spaces, thus indenting my code too much.

To be honest I don't know how would that be translated to the final code since when I open it on my text editor they seem to be indented the right way.

I will let you and Simarpreet know when I have further progress.

Comment 9

[Camilo]

Attachment: log_module_docstring.patch

Complete documentation of the log.py module on mozharness base.

Comment 10

[Simarpreet Singh]

Comment on attachment 8603403 [details] [diff] [review]
log_module_docstring.patch

>diff --git a/mozharness/base/log.py b/mozharness/base/log.py
>index 218d9a9..dab68b7 100755
>--- a/mozharness/base/log.py
>+++ b/mozharness/base/log.py
>@@ -4,7 +4,20 @@
> # License, v. 2.0. If a copy of the MPL was not distributed with this file,
> # You can obtain one at http://mozilla.org/MPL/2.0/.
> # ***** END LICENSE BLOCK *****
>-"""Generic logging, the way I remember it from scripts gone by.
>+"""Generic logging classes and functionalities for single and multi file logging.
>+Capturing console output and providing general logging functionalities.
>+
>+Attributes:
>+	FATAL_LEVEL (int): constant logging level value set based on the logging.CRITICAL
>+		value
>+	DEBUG (str): mozharness `debug` log name
>+	INFO (str): mozharness `info` log name
>+	WARNING (str): mozharness `warning` log name
>+	CRITICAL (str): mozharness `critical` log name
>+	FATAL (str): mozharness `fatal` log name
>+	IGNORE (str): mozharness `ignore` log name
>+	LOG_LEVELS (dict): mapping of the mozharness log level names to logging values
>+	ROOT_LOGGER (logging.Logger): instance of a logging.Logger class
> 
> TODO:
> - network logging support.
>@@ -41,12 +54,21 @@ ROOT_LOGGER = logging.getLogger()
> 
> # LogMixin {{{1
> class LogMixin(object):
>-    """This is a mixin for any object to access similar logging
>-    functionality -- more so, of course, for those objects with
>-    self.config and self.log_obj, of course.
>+    """This is a mixin for any object to access similar logging functionality
>+
>+	The logging functionality described here is specially useful for those objects
>+	with self.config and self.log_obj member variables
>     """
> 
>     def _log_level_at_least(self, level):
>+		""" Check if the current logging level is greater or equal than level
>+		Args:
>+			level (str): log level name to compare against mozharness log levels names
>+
>+		Returns:
>+			bool: True if the current loggin level is great or equal than level,
>+				 False otherwise
>+		"""
>         log_level = INFO
>         levels = [DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL]
>         if hasattr(self, 'config'):
>@@ -54,6 +76,17 @@ class LogMixin(object):
>         return levels.index(level) >= levels.index(log_level)
> 
>     def _print(self, message, stderr=False):
>+		""" prints a message to the sys.stdout or sys.stderr according to the value of
>+		the stderr argument.
>+
>+		Args:
>+			message (str): The message to be printed
>+			stderr (bool, optional): if True, message will be printed to sys.stderr.
>+				Defaults to False.
>+
>+		Returns:
>+			None
>+		"""
>         if not hasattr(self, 'config') or self.config.get('log_to_console', True):
>             if stderr:
>                 print >> sys.stderr, message
>@@ -61,6 +94,17 @@ class LogMixin(object):
>                 print message
> 
>     def log(self, message, level=INFO, exit_code=-1):
>+		""" log the message passed to it according to level, exit if level == FATAL
>+
>+		Args:
>+			message (str): message to be logged
>+			level (str, optional): logging level of the message. Defaults to INFO
>+			exit_code (int, optional): exit code to log before the scripts calls
>+				SystemExit.
>+
>+		Returns:
>+			None
>+		"""
>         if self.log_obj:
>             return self.log_obj.log_message(
>                 message, level=level,
>@@ -82,9 +126,21 @@ class LogMixin(object):
>             raise SystemExit(exit_code)
> 
>     def worst_level(self, target_level, existing_level, levels=None):
>-        """returns either existing_level or target level.
>-        This depends on which is closest to levels[0]
>-        By default, levels is the list of log levels"""
>+        """Compare target_level with existing_level according to levels values
>+		and return the worst among them.
>+
>+		Args:
>+			target_level (str): minimum logging level to which the current object
>+				should be set
>+			existing_level (str): current logging level
>+			levels (list(str), optional): list of logging levels names to compare
>+				target_level and existing_level against. Defaults to mozharness log level
>+				list sorted from most to less critical.
>+
>+		Returns:
>+			str: the logging lavel that is closest to the first levels value,
>+				i.e. levels[0]
>+		"""
>         if not levels:
>             levels = [FATAL, CRITICAL, ERROR, WARNING, INFO, DEBUG, IGNORE]
>         if target_level not in levels:
>@@ -96,6 +152,19 @@ class LogMixin(object):
>     # Copying Bear's dumpException():
>     # https://hg.mozilla.org/build/tools/annotate/1485f23c38e0/sut_tools/sut_lib.py#l23
>     def exception(self, message=None, level=ERROR):
>+		""" log an exception message base on the log level passed to it.
>+
>+		This function fetches the information of the current exception being handled and
>+		adds it to the message argument.
>+
>+		Args:
>+			message (str, optional): message to be printed at the beginning of the log.
>+				Default to an empty string.
>+			level (str, optional): log level to use for the logging. Defaults to ERROR
>+
>+		Returns:
>+			None
>+		"""
>         tb_type, tb_value, tb_traceback = sys.exc_info()
>         if message is None:
>             message = ""
>@@ -107,28 +176,65 @@ class LogMixin(object):
>         self.log(message, level=level)
> 
>     def debug(self, message):
>+		""" calls the log method with DEBUG as logging level
>+
>+		Args:
>+			message (str): message to log
>+		"""
>         self.log(message, level=DEBUG)
> 
>     def info(self, message):
>+		""" calls the log method with INFO as logging level
>+
>+		Args:
>+			message (str): message to log
>+		"""
>         self.log(message, level=INFO)
> 
>     def warning(self, message):
>+		""" calls the log method with WARNING as logging level
>+
>+		Args:
>+			message (str): message to log
>+		"""
>         self.log(message, level=WARNING)
> 
>     def error(self, message):
>+		""" calls the log method with ERROR as logging level
>+
>+		Args:
>+			message (str): message to log
>+		"""
>         self.log(message, level=ERROR)
> 
>     def critical(self, message):
>+		""" calls the log method with CRITICAL as logging level
>+
>+		Args:
>+			message (str): message to log
>+		"""
>         self.log(message, level=CRITICAL)
> 
>     def fatal(self, message, exit_code=-1):
>+		""" calls the log method with FATAL as logging level
>+
>+		Args:
>+			message (str): message to log
>+			exit_code (int, optional): exit code to use for the SystemExit exception
>+				to be raised. Default to -1.
>+		"""
>         self.log(message, level=FATAL, exit_code=exit_code)
> 
>     def _post_fatal(self, message=None, exit_code=None):
>         """ Sometimes you want to create a report or cleanup
>-            or notify on fatal(); override this method to do so.
>+		or notify on fatal(); override this method to do so.
>+
>+        Please don't use this for anything significantly long-running.
> 
>-            Please don't use this for anything significantly long-running.
>+		Args:
>+			message (str, optional): message to report. Default to None
>+			exit_code (int, optional): exit code to use for the SystemExit exception
>+				to be raised. Default to None
Any reason why this args section is intended differently?
>         """
>         pass
> 
>@@ -137,23 +243,36 @@ class LogMixin(object):
> class OutputParser(LogMixin):
>     """ Helper object to parse command output.
> 
>-This will buffer output if needed, so we can go back and mark
>-[(linenum - 10):linenum+10] as errors if need be, without having to
>-get all the output first.
>-
>-linenum+10 will be easy; we can set self.num_post_context_lines to 10,
>-and self.num_post_context_lines-- as we mark each line to at least error
>-level X.
>-
>-linenum-10 will be trickier. We'll not only need to save the line
>-itself, but also the level that we've set for that line previously,
>-whether by matching on that line, or by a previous line's context.
>-We should only log that line if all output has ended (self.finish() ?);
>-otherwise store a list of dictionaries in self.context_buffer that is
>-buffered up to self.num_pre_context_lines (set to the largest
>-pre-context-line setting in error_list.)
>-"""
>+	This will buffer output if needed, so we can go back and mark
>+	[(linenum - 10) : linenum+10] as errors if need be, without having to
>+	get all the output first.
>+
>+	linenum+10 will be easy; we can set self.num_post_context_lines to 10,
>+	and self.num_post_context_lines-- as we mark each line to at least error
>+	level X.
>+
>+	linenum-10 will be trickier. We'll not only need to save the line
>+	itself, but also the level that we've set for that line previously,
>+	whether by matching on that line, or by a previous line's context.
>+	We should only log that line if all output has ended (self.finish() ?);
>+	otherwise store a list of dictionaries in self.context_buffer that is
>+	buffered up to self.num_pre_context_lines (set to the largest
>+	pre-context-line setting in error_list.)
>+	"""
>+
>     def __init__(self, config=None, log_obj=None, error_list=None, log_output=True):
>+		"""Initialization method for the OutputParser class
>+
>+		Args:
>+			config (dict, optional): dictionary containing values such as `log_level`
>+				or `log_to_console`. Defaults to `None`.
>+			log_obj (BaseLogger, optional): instance of the BaseLogger class. Defaults
>+				to `None`.
>+			error_list (list, optional): list of the error to look for. Defaults to
>+				and `None`.
>+			log_output (boolean, optional): flag for deciding if the commands output
>+				should be logged or not. Defaults to `True`.
>+		"""
>         self.config = config
>         self.log_obj = log_obj
>         self.error_list = error_list or []
>@@ -168,6 +287,12 @@ pre-context-line setting in error_list.)
>         self.worst_log_level = INFO
> 
>     def parse_single_line(self, line):
>+		""" parse a console output line and check if it matches one in `error_list`,
>+		if so then log it according to `log_output`.
>+
>+		Args:
>+			line (str): command line output to parse.
>+		"""
>         for error_check in self.error_list:
>             # TODO buffer for context_lines.
>             match = False
>@@ -202,6 +327,14 @@ pre-context-line setting in error_list.)
>                 self.info(' %s' % line)
> 
>     def add_lines(self, output):
>+		""" process a string or list of strings, decode them to utf-8,strip
>+		them of any trailing whitespaces and parse them using `parse_single_line`
>+
>+		strings consisting only of whitespaces are ignored.
>+		Args:
>+			output (str | list): string or list of string to parse
>+		"""
>+
>         if isinstance(output, basestring):
>             output = [output]
>         for line in output:
>@@ -213,7 +346,12 @@ pre-context-line setting in error_list.)
> 
> # BaseLogger {{{1
> class BaseLogger(object):
>-    """Create a base logging class.
>+    """ Base class in charge of logging handling logic such as creating logging
>+	files, dirs, attaching to the console output and managing its output.
>+
>+	Attributes:
>+		LEVELS (dict): flat copy of the `LOG_LEVELS` attribute of the `log` module.
>+
>     TODO: status? There may be a status object or status capability in
>     either logging or config that allows you to count the number of
>     error,critical,fatal messages for us to count up at the end (aiming
>@@ -232,6 +370,29 @@ class BaseLogger(object):
>         logger_name='',
>         append_to_log=False,
>     ):
>+		""" BaseLogger constructor
>+
>+		Args:
>+			log_level (str, optional): mozharness log level name. Defaults to INFO.
>+			log_format (str, optional): message format string to instantiate a
>+				`logging.Formater`. Defaults to '%(message)s'
>+			log_date_format (str, optional): date format string to instantiate a
>+				`logging.Formater`. Defaults to '%H:%M:%S'
>+			log_name (str, optional): name to use for the log files to be created.
>+				Defaults to 'test'
>+			log_to_console (bool, optional): set to True in order to create a Handler
>+				instance base on the `Logger` current instance. Defaults to True.
>+			log_dir (str, optional): directory location to store the log files.
>+				Defaults to '.', i.e. current working directory.
>+			log_to_raw (bool, optional): set to True in order to create a *raw.log
>+				file. Defaults to False.
>+			logger_name (str, optional): currently useless parameter.
>+				According to the code comments, it could be useful if we were to have
>+				multiple logging objects that don't trample each other.
>+			append_to_log (bool, optional): set to True if the logging content should
>+				be appended to old logging files. Defaults to False
>+		"""
>+
>         self.log_format = log_format
>         self.log_date_format = log_date_format
>         self.log_to_console = log_to_console
>@@ -251,6 +412,10 @@ class BaseLogger(object):
>         self.create_log_dir()
> 
>     def create_log_dir(self):
>+		""" create a logging directory if it doesn't exits. If there is a file with
>+		same name as the future logging directory it will be deleted.
>+		"""
>+
>         if os.path.exists(self.log_dir):
>             if not os.path.isdir(self.log_dir):
>                 os.remove(self.log_dir)
>@@ -259,6 +424,14 @@ class BaseLogger(object):
>         self.abs_log_dir = os.path.abspath(self.log_dir)
> 
>     def init_message(self, name=None):
>+		""" log an init message stating the name passed to it, the current date
>+		and time and, the current working directory.
>+
>+		Args:
>+			name (str, optional): name to use for the init log message. Defaults to
>+			the current instance class name.
>+		"""
>+
>         if not name:
>             name = self.__class__.__name__
>         self.log_message("%s online at %s in %s" %
>@@ -266,11 +439,35 @@ class BaseLogger(object):
>                           os.getcwd()))
> 
>     def get_logger_level(self, level=None):
>+		""" translate the level name passed to it and return its numeric value
>+		according to `LEVELS` values.
>+
>+		Args:
>+			level (str, optional): level name to be translated. Defaults to the current
>+				instance `log_level`.
>+
>+		Returns:
>+			int: numeric value of the log level name passed to it or 0 (NOTSET) if the
>+				name doesn't exists
>+		"""
>+
>         if not level:
>             level = self.log_level
>         return self.LEVELS.get(level, logging.NOTSET)
> 
>     def get_log_formatter(self, log_format=None, date_format=None):
>+		""" create a `logging.Formatter` base on the log and date format.
>+
>+		Args:
>+			log_format (str, optional): log format to use for the Formatter constructor.
>+				Defaults to the current instance log format.
>+			date_format (str, optional): date format to use for the Formatter constructor.
>+				Defaults to the current instance date format.
>+
>+		Returns:
>+			logging.Formater: instance created base on the passed arguments
Typo in Formatter.

>+		"""
>+
>         if not log_format:
>             log_format = self.log_format
>         if not date_format:
>@@ -278,9 +475,10 @@ class BaseLogger(object):
>         return logging.Formatter(log_format, date_format)
> 
>     def new_logger(self):
>-        """Create a new logger.
>-        By default there are no handlers.
>-        """
>+		""" Create a new logger based on the ROOT_LOGGER instance. By default there are no handlers.
>+			The new logger becomes a member variable of the current instance as `self.logger`.
>+		"""
>+
>         self.logger = ROOT_LOGGER
>         self.logger.setLevel(self.get_logger_level())
>         self._clear_handlers()
>@@ -293,7 +491,9 @@ class BaseLogger(object):
>                                   log_format='%(message)s')
> 
>     def _clear_handlers(self):
>-        """To prevent dups -- logging will preserve Handlers across
>+        """ remove all handlers stored in `self.all_handlers`.
>+
>+		To prevent dups -- logging will preserve Handlers across
>         objects :(
>         """
>         attrs = dir(self)
>@@ -303,11 +503,25 @@ class BaseLogger(object):
>             self.all_handlers = []
> 
>     def __del__(self):
>+		""" BaseLogger class destructor; shutdown, flush and remove all handlers
>+        """
>         logging.shutdown()
>         self._clear_handlers()
> 
>     def add_console_handler(self, log_level=None, log_format=None,
>                             date_format=None):
>+		""" create a `logging.StreamHandler` using `sys.stderr` for logging the console
>+		output and add it to the `all_handlers` member variable
>+
>+		Args:
>+			log_level (str, optional): useless argument. Not used here.
>+				Defaults to None.
>+			log_format (str, optional): format used for the Formatter attached to the
>+				StreamHandler. Defauls to None.
typo in Defaults.
>+			date_format (str, optional): format used for the Formatter attached to the
>+				StreamHandler. Defaults to None.
>+		"""
>+
>         console_handler = logging.StreamHandler()
>         console_handler.setFormatter(self.get_log_formatter(log_format=log_format,
>                                                             date_format=date_format))
>@@ -316,6 +530,19 @@ class BaseLogger(object):
> 
>     def add_file_handler(self, log_path, log_level=None, log_format=None,
>                          date_format=None):
>+		""" create a `logging.FileHandler` base on the path, log and date format
>+		and add it to the `all_handlers` member variable.
>+
>+		Args:
>+			log_path (str): filepath to use for the `FileHandler`.
>+			log_level (str, optional): useless argument. Not used here.
>+				Defaults to None.
>+			log_format (str, optional): log format to use for the Formatter constructor.
>+				Defaults to the current instance log format.
>+			date_format (str, optional): date format to use for the Formatter constructor.
>+				Defaults to the current instance date format.
>+		"""
>+
>         if not self.append_to_log and os.path.exists(log_path):
>             os.remove(log_path)
>         file_handler = logging.FileHandler(log_path)
>@@ -325,13 +552,22 @@ class BaseLogger(object):
>         self.all_handlers.append(file_handler)
> 
>     def log_message(self, message, level=INFO, exit_code=-1, post_fatal_callback=None):
>-        """Generic log method.
>+		""" Generic log method.
>         There should be more options here -- do or don't split by line,
>         use os.linesep instead of assuming \n, be able to pass in log level
>         by name or number.
> 
>         Adding the IGNORE special level for runCommand.
>-        """
>+
>+		Args:
>+			message (str): message to log using the current `logger`
>+			level (str, optional): log level of the message. Defaults to INFO.
>+			exit_code (int, optional): exit code to use in case of a FATAL level is used.
>+				Defaults to -1.
>+			post_fatal_callback (function, optional): function to callback in case of
>+				of a fatal log level. Defaults None.
>+		"""
>+
>         if level == IGNORE:
>             return
>         for line in message.splitlines():
>@@ -346,18 +582,37 @@ class BaseLogger(object):
> 
> # SimpleFileLogger {{{1
> class SimpleFileLogger(BaseLogger):
>-    """Create one logFile.  Possibly also output to
>-    the terminal and a raw log (no prepending of level or date)
>-    """
>+    """ Subclass of the BaseLogger.
>+
>+	Create one logFile.  Possibly also output to the terminal and a raw log
>+	(no prepending of level or date)
>+	"""
>+
>     def __init__(self,
>                  log_format='%(asctime)s %(levelname)8s - %(message)s',
>                  logger_name='Simple', log_dir='logs', **kwargs):
>+		""" SimpleFileLogger constructor. Calls its superclass constructor,
>+		creates a new logger instance and log an init message.
>+
>+		Args:
>+			log_format (str, optional): message format string to instantiate a
>+				`logging.Formater`. Defaults to '%(asctime)s %(levelname)8s - %(message)s'
Typo in Formatter.
>+			log_name (str, optional): name to use for the log files to be created.
>+				Defaults to 'Simple'
>+			log_dir (str, optional): directory location to store the log files.
>+				Defaults to 'logs'
>+			**kwargs: Arbitrary keyword arguments passed to the BaseLogger constructor
>+		"""
>+
>         BaseLogger.__init__(self, logger_name=logger_name, log_format=log_format,
>                             log_dir=log_dir, **kwargs)
>         self.new_logger()
>         self.init_message()
> 
>     def new_logger(self):
>+		""" calls the BaseLogger.new_logger method and adds a file handler to it.
>+		"""
>+
>         BaseLogger.new_logger(self)
>         self.log_path = os.path.join(self.abs_log_dir, '%s.log' % self.log_name)
>         self.log_files['default'] = self.log_path
>@@ -366,12 +621,27 @@ class SimpleFileLogger(BaseLogger):
> 
> # MultiFileLogger {{{1
> class MultiFileLogger(BaseLogger):
>-    """Create a log per log level in log_dir.  Possibly also output to
>-    the terminal and a raw log (no prepending of level or date)
>+    """Subclass of the BaseLogger class. Create a log per log level in log_dir.
>+	Possibly also output to the terminal and a raw log (no prepending of level or date)
>     """
>     def __init__(self, logger_name='Multi',
>                  log_format='%(asctime)s %(levelname)8s - %(message)s',
>                  log_dir='logs', log_to_raw=True, **kwargs):
>+		""" MultiFileLogger constructor. Calls its superclass constructor,
>+		creates a new logger instance and log an init message.
>+
>+		Args:
>+			log_format (str, optional): message format string to instantiate a
>+				`logging.Formater`. Defaults to '%(asctime)s %(levelname)8s - %(message)s'
Typo in Formatter
>+			log_name (str, optional): name to use for the log files to be created.
>+				Defaults to 'Multi'
>+			log_dir (str, optional): directory location to store the log files.
>+				Defaults to 'logs'
>+			og_to_raw (bool, optional): set to True in order to create a *raw.log
>+				file. Defaults to False.
>+			**kwargs: Arbitrary keyword arguments passed to the BaseLogger constructor
>+		"""
>+
>         BaseLogger.__init__(self, logger_name=logger_name,
>                             log_format=log_format,
>                             log_to_raw=log_to_raw, log_dir=log_dir,
>@@ -381,6 +651,10 @@ class MultiFileLogger(BaseLogger):
>         self.init_message()
> 
>     def new_logger(self):
>+		""" calls the BaseLogger.new_logger method and adds a file handler per
>+		logging level in the `LEVELS` class attribute.
>+		"""
>+
>         BaseLogger.new_logger(self)
>         min_logger_level = self.get_logger_level(self.log_level)
>         for level in self.LEVELS.keys():
>@@ -393,12 +667,19 @@ class MultiFileLogger(BaseLogger):
> 
> 
> def numeric_log_level(level):
>-    """Converts a mozharness log level (string) to the corresponding logger
>-       level (number). This function makes possible to set the log level
>-       in functions that do not inherit from LogMixin
>+	"""Converts a mozharness log level (string) to the corresponding logger
>+    level (number). This function makes possible to set the log level
>+    in functions that do not inherit from LogMixin
>+
>+	Args:
>+		level (str): log level name to convert.
>+
>+	Returns:
>+		int: numeric value of the log level name.
>     """
>     return LOG_LEVELS[level]
> 
> # __main__ {{{1
> if __name__ == '__main__':
>+	""" Useless comparison, due to the `pass` keyword on its body"""
>     pass

Overall looks good, thanks for the contribution. Please fix the necessary things and I can re-review it again.

Comment 11

[Camilo]

Attachment: log_module_docstring.patch

Typos corrected.

Regarding the Args section, to be honest I don't know. In my text editor they look ok, but I do recognize that in the patch the Args sections seems to have moved a bit o.O

PS: Simarpreet, could you please make the code snippets a bit shorter to make it easy to recognize about which part are you talking about?

Comment 12

[Camilo]

Comment on attachment 8603823 [details] [diff] [review]
log_module_docstring.patch

Hey Armen,
could you review this patch please, I just received an overdue request from Simarpreet :/

Comment 13

[Armen [:armenzg]]

Comment on attachment 8603823 [details] [diff] [review]
log_module_docstring.patch

Review of attachment 8603823 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Camilo,
Your attention to details is outstanding. You worded everything very well and followed a consistent pattern.
This is very well done; kudos!

BTW, under an attachment you can see the word "Review" which takes you to a splinter view of the code.
You can also see the comments I'm about to make.
You can reply to my comments if you need to by double clicking under my comments.
Just for the giggles you can open it and see what I mean.

For the record, all comment blocks for all functions are shifted 8 characters to the right. I will point out one of them.
Once you shift it to the left, could you see which of the lines on those comments can then be put back together? Right now, some of them are overflowing into a new line.
What editor do you use? I use Vim.

Please attach a new patch fixing the indenting issue and typo. If this gets fixed I will land the next patch on your behalf.

I think it is introducing tabs instead of white spaces, if you select the white spaces on the following text with the mouse you can tell that there are 2 or more tabs:
     def _log_level_at_least(self, level):
+		""" Check if the current logging level is greater or equal than level
+		Args:
+			level (str): log level name to compare against mozharness log levels names
+
+		Returns:
+			bool: True if the current loggin level is great or equal than level,
+				 False otherwise
+		"""
         log_level = INFO

::: mozharness/base/log.py
@@ +8,5 @@
> +Capturing console output and providing general logging functionalities.
> +
> +Attributes:
> +	FATAL_LEVEL (int): constant logging level value set based on the logging.CRITICAL
> +		value

Would you mind either putting the word 'value' on the previous line or line it up visually under the word 'constant'?

@@ +61,4 @@
>      """
>  
>      def _log_level_at_least(self, level):
> +		""" Check if the current logging level is greater or equal than level

Shifted 8 characters to the right.

@@ +65,5 @@
> +		Args:
> +			level (str): log level name to compare against mozharness log levels names
> +
> +		Returns:
> +			bool: True if the current loggin level is great or equal than level,

s/loggin/logging/

Comment 14

[Camilo]

Attachment: log_py_docstring.patch

- Indentation fixed from tabs to spaces.
- docstrings line continuation fixed to follow the level of the previous line.

Armen,
I couldn't understand your last comment: s/logging/logging?
What does it mean?

Comment 15

[Armen [:armenzg]]

(In reply to Camilo from comment #14)
> 
> Armen,
> I couldn't understand your last comment: s/logging/logging?
> What does it mean?

My bad. It is short for "substitute word1 for word2"
That should have read:
s/loggin/logging

There was a missing 'g' in the word. I can fix it when I land your patch if we don't require any more patches.

Comment 16

[Armen [:armenzg]]

Comment on attachment 8611357 [details] [diff] [review]
log_py_docstring.patch

Review of attachment 8611357 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/build/mozharness/rev/d7bf2a173f45

Comment 17

[Armen [:armenzg]]

Camilo, please let me know if you have interst on working on more mozharness code or you would like to try something else.

If the first, I will be having a bit of trouble mentoring since I'm not involved with mozharness as much, however, I can propose other projects including one [1] that I'm mentoring.

[1] https://github.com/armenzg/mozilla_ci_tools

Comment 18

[Armen [:armenzg]]

<armenzg> jlund, do you have any mozharness contributions that Camilo could take?
<jlund> armenzg_brb: off a glance, https://bugzil.la/1133685 https://bugzil.la/1143850 https://bugzil.la/1161556 https://bugzil.la/1127449 https://bugzil.la/1162230

Comment 19

[Camilo]

Hey Armen,
thanks for the trust =D

Nevertheless, next week I will start my internship so I will probably not have so much time to actively contribute to coding tasks.

That is precisely the reason why I decided to work on the documentation instead of going for coding. For the docstrings, as long as I understand the python calls I don't need to test nor care about CI, which I think will be quite helpful when I start my internship since I won't have so much free time anymore (not to talk about mental tiredness)

I checked all the bugs that you referred me to on #comment 18, and from my point of view somebody can solve them more easily if they can understand what the code is really doing (docstring) instead of just guessing as was stated in several descriptions.

(In reply to Armen from comment #17)
>
> Camilo, please let me know if you have interst on working on more mozharness code or you would like > to try something else.
> 
> If the first, I will be having a bit of trouble mentoring since I'm not involved with mozharness as
> much, however, I can propose other projects including one [1] that I'm mentoring.

The project seems to be quite interesting, but the same reasons showed above apply. Nevertheless, it would be nice if you could relate me to somebody working on mozharness that can answer some questions every now and then.

Maybe in a future I will be able to take some of those tasks ;)

Comment 20

[Armen [:armenzg]]

Fair enough!
It's very much appreciated your help and looking forward to what the future may hold :)

Comment 21

[Camilo]

Attachment: script_py_docstring.patch

Hey Armen,
since this file is quite big. Would it be possible to do a two (or more) steps review ?

Comment 22

[Armen [:armenzg]]

Comment on attachment 8612458 [details] [diff] [review]
script_py_docstring.patch

Review of attachment 8612458 [details] [diff] [review]:
-----------------------------------------------------------------

Fine for two steps review.
I can do something called inter-diff to see what changes from the first patch to the next one.

Great job with this first one.
Can you please address the mentioned issues?

FYI, I will be using f+ to indicate that I'm happy with the results and only r+ meaning that this is ready to land and I will take care of it.

::: mozharness/base/script.py
@@ +57,4 @@
>      together with logging and config, provide the base for all scripts
>      in this harness.
>  
> +    ¡¡¡ WARNING !!!

I love the Spanish left exclamation marks (long time that I have not seen them :D), however, most people looking at this code will be surprised to see them. Could you please remove them?

@@ +82,5 @@
> +            path (str): path of the directory to be created.
> +            error_level (str): log level name to be used in case of error.
> +
> +        Returns:
> +            None: for sucess.

Any reason we don't mention what the failure return is?

@@ +99,3 @@
>  
>      def rmtree(self, path, log_level=INFO, error_level=ERROR,
>                 exit_code=-1):

I don't see exit_code being used anymore. Could you please update this signature and the associated comment below?

@@ +152,4 @@
>              self.debug("%s doesn't exist." % path)
>  
>      def _is_windows(self):
> +        """ check if the currennt operating system is Windows.

'current' has a typo. The same issue on the following functions.

@@ +191,5 @@
>  
>      def query_msys_path(self, path):
> +        """ Unknown method, not used in any script.
> +        Apparently it replaces the Windows harddrive letter path style with a linux
> +        path style, e.g. C:// --> /C/

I hear you. Can you please worded it without "Unknown..." and "Apparently"?
You got the logic right.

@@ +194,5 @@
> +        Apparently it replaces the Windows harddrive letter path style with a linux
> +        path style, e.g. C:// --> /C/
> +
> +        Args:
> +            path (str?): path to convert to the linux path style.

No need for question mark.

@@ +220,5 @@
> +        Returns:
> +            None: if the path doesn't exists.
> +            int: the return number of calling `self.run_command`
> +            unknown: in case the path specified is not a directory but a file.
> +                     The returned value is then the result of calling `win32file.DeleteFile`

Return explained in here:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363915%28v=vs.85%29.aspx

@@ +1709,4 @@
>  
>  # __main__ {{{1
>  if __name__ == '__main__':
> +	""" Useless comparison, due to the `pass` keyword on its body"""

This is extra indented.

Comment 23

[Armen [:armenzg]]

Camilo, would you prefer contributing to the future replacement of mozharness?

It's called python-scriptharness
https://github.com/scriptharness/python-scriptharness

I think it has more value to contribute documentation in there since there is even auto-generated documenation for it:
http://python-scriptharness.readthedocs.org/en/latest/scriptharness.script

I assume scriptharness will be more widely used by other communities since its been better designed from the ground up towards it.

What do you think?

Aki (escapedwindow) wrote the original mozharness, however, he's now focusing on the replacement and we might switch to that in the future.

Comment 24

[Camilo]

Hey Armen,
well it sounds like a nice idea. But is it a fact that it will be the replacement of  mozharness (provided that it gets fully developed obviously) ? Or is it more or of a "we will see ..."
because I would personally not like to invest time (that I'm voluntarily giving) to something that will never come to see the light.

In case that python-scriptharness would in fact be the proposed replacement for mozharness I would be happy to contribute :)
In that case, how should we do it with the current mozharness documentation? should I finish this file (script.py) and directly switch to scriptharness?

Comment 25

[Armen [:armenzg]]

Hi Camilo,
I wish I knew the answer to those questions.

As far as I know, we don't auto-generate the mozharness documentation for people to reference (e.g. readthedocs or anything like that).
They will get the value of your work by looking at the source code when writing new code.
In fact, I already see personal value by knowing the types of the input variable and output variables.
We use mozharness every day at Mozilla for everything that runs on treeherder.mozilla.org
I can not foresee that we would switch from mozharness to python-mozharness until maybe next year if it made sense. Let's say we decide to port all of our scripts to Python 3 and python-scriptharness gives us better value.

With regards to python-scriptharness, I don't know who out there on the open source world would use it. No consumers atm from what I understand.

I'm happy to receive your doc contributions here if you would like to, however, I support either way what you prefer.


The only docs that I see about mozharness are in here:
http://moz-releng-docs.readthedocs.org/en/latest/software.html#mozharness

Comment 26

[Camilo]

Hey Armen,
after checking a bit more about scriptharness, I decided that I will support it with the documentation part. It seems to address some of the concerns that I also had about mozharness, so I will help it have a documented start ;)

I saw that Aki is no longer in mozilla, should I directly contact him or rather through bugzilla?

Regarding mozharness, how should we do it? In my opinion, I guess I should finish the class that I was doing the documentation for, so that the important part of mozharness is documented. Or rather better leaving it as it is?

PS: By coincidence I ran into this website quite some time ago ;)
http://moz-releng-mozharness.readthedocs.org/en/latest/index.html

Comment 27

[Armen [:armenzg]]

Oh wow! I didn't know we had docs published!

If you want to finish what you started I'm happy to review it but either way works for me.

Aki is pretty active on GH or you can reach him here. I think GH is preferable since scriptharness does not use BZ for tracking issues.
I'm sure he will see the pull requests.

Comment 28

[Aki Sasaki (not active)]

In theory, all of scriptharness should be docstringed atm.

Comment 29

[Camilo]

Comment on attachment 8612458 [details] [diff] [review]
script_py_docstring.patch

Review of attachment 8612458 [details] [diff] [review]:
-----------------------------------------------------------------

Regarding the indentation, after seeing how was the code being outputed to html I discover that most likely the indentation is making troubles for multiline descriptions. Due to that, I will make the rest of the docstring with that new indentation style to avoid that problem
For example:
path (str): this is a description that has
  a multiline description.

::: mozharness/base/script.py
@@ +194,5 @@
> +        Apparently it replaces the Windows harddrive letter path style with a linux
> +        path style, e.g. C:// --> /C/
> +
> +        Args:
> +            path (str?): path to convert to the linux path style.

hey Armen,
in the code there is an if condition checking that the path is of type str. Therefore I guessed that `path` can be of a different type. The question mark was the only way that I found to express such a relation.

Comment 30

[Camilo]

Attachment: script_py_docstring.patch

Hey Armen,
This will be my last patch regarding the documentation of mozharness. I tried to cover the most important part of mozharness base in case somebody wants to understand it more. The rest of the documentation should be easier for somebody else if that happens before you switch to scriptharness ;)

I briefly talked with Aki and I will update parts of the documentation there. So that will be my focus now instead of mozharness.

Let me know if there are any errors

Comment 31

[Armen [:armenzg]]

Comment on attachment 8623841 [details] [diff] [review]
script_py_docstring.patch

Review of attachment 8623841 [details] [diff] [review]:
-----------------------------------------------------------------

Camilo, this is excellent work. You've got the chops!

I'm glad you got to talk with Aki and I'm happy to see that he could receive of your fine contributions!

Comment 32

[Armen [:armenzg]]

If anything left, we can direct the contributions to scriptharness:
https://hg.mozilla.org/build/mozharness/rev/b773d51ca318