Dictionary for log messages #55

Christian-B · 2018-04-26T14:18:40Z

Part of SpiNNakerManchester/spinnaker_tools#75
Does not break if merged earlier

--
Code to convert c files by replacing log_ messages

As well as code to put it back.

This reverts commit dc8fc5b.

coveralls · 2018-05-01T08:17:28Z

Pull Request Test Coverage Report for Build 566

347 of 401 (86.53%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.2%) to 91.667%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spinn_utilities/make_tools/replacer.py	34	38	89.47%
spinn_utilities/executable_finder.py	1	7	14.29%
spinn_utilities/make_tools/converter.py	70	89	78.65%
spinn_utilities/make_tools/file_converter.py	240	265	90.57%

Totals
Change from base Build 540:	-1.2%
Covered Lines:	1914
Relevant Lines:	2088

💛 - Coveralls

alan-stokes · 2018-05-29T08:39:56Z

spinn_utilities/make_tools/__init__.py

+from .file_convertor import FileConvertor
+
+__all__ = [
+    "FileConvertor"]


converter vs convertor ???

Ug the pain when even @alan-stokes notices spelling mistakes.

Fixed

alan-stokes · 2018-05-29T08:41:06Z

spinn_utilities/make_tools/convertor.py

+
+class Convertor(object):
+    # __slots__ = [
+    #    "_dest", "_dest_basename", "_src", "_src_basename"]


why commented out?

Planned on doing when I stopped changing my mine and then forgot.

Fixed

alan-stokes · 2018-05-29T08:55:32Z

spinn_utilities/make_tools/convertor.py

+    # __slots__ = [
+    #    "_dest", "_dest_basename", "_src", "_src_basename"]
+
+    def __init__(self, src, dest, dict):


docs on what these params are would be useful. plus its a constructor, docs on its inputs always usful

alan-stokes · 2018-05-29T08:58:58Z

spinn_utilities/make_tools/convertor.py

+                    print("Unexpected file {}".format(source))
+
+    def _get_id(self):
+        RANGE_PER_DIR = 1000


magic number inside function, push to class level.

alan-stokes · 2018-05-29T09:00:07Z

spinn_utilities/make_tools/convertor.py

+            raise Exception("mkdir failed {}".format(destination))
+
+    def _find_common_based_on_environ(self):
+        if 'SPINN_DIRS' not in os.environ:


make into magic_param as then if we ever change it, you only need change it in one place. this method has 2 of each already

"{} {}\n" Here I disagree with making this a CONSTANT or even a magic. Keep the line needing parameters with the code that provides them.

alan-stokes · 2018-05-29T09:00:54Z

spinn_utilities/make_tools/convertor.py

+    def run(self):
+        self._mkdir(self._dest)
+        with open(self._dict, 'w') as dict_f:
+            dict_f.write("Id,Preface,Original\n"


create text as magic param

there called CONSTANTS

alan-stokes · 2018-05-29T09:01:43Z

spinn_utilities/make_tools/convertor.py

+    def _get_id(self):
+        RANGE_PER_DIR = 1000
+
+        rangefile = os.path.join(RANGE_DIR, "log.ranges")


log.ranges (magic name?)

Added CONSTANT as don't do magic

alan-stokes · 2018-05-29T09:12:47Z

spinn_utilities/make_tools/file_convertor.py

+                self._too_many_lines = 2
+                self._status = NORMAL_CODE
+                for self._line_num, self._text in enumerate(src_f):
+                    if self._too_many_lines > 0:


self._too_many_lines not created during constructor, nor in slots

same for _status, _line_num, _test, _comment_start,

added to slots. CRITICAL!

Did not add to init as I actually prefer the never created error over the null pointer error.

phft to you. ill bring in rowley tomorrow and we can have a mediator on whom is right

A quick check shows that too_many_lines is set on line 60 then used in the next few lines within the same function. There is one reference outside the function where this variable is added to - however this addition seems to then be ignored... this seems incorrect; the variable can probably either be local or should be dealt with correctly later.

_text looks lime it should be passed in to the processing functions - this makes tracing it easier and the code easier to follow.

Looks like _line_num can also be passed down?

_comment_start looks to be write-only

too_many_lines is added to in
_write_log_method which is called from
_process_line_in_log (and others) which is called from
_process_line (and others) which is called from
inside the for loop which does skip lines in based on -_too_many_lines

alan-stokes · 2018-05-29T09:23:51Z

spinn_utilities/make_tools/file_convertor.py

+        but so far falls over on a TOKEN in a /* */ style comment.
+        If needed could be altered to handle those as well.
+
+        Also note that is is just a safety check and not stricktly required.


strictly vs stricktly

nuked whole method

alan-stokes · 2018-05-29T09:25:04Z

spinn_utilities/make_tools/file_convertor.py

+import re
+import sys
+
+TOKEN = chr(30)  # Record Sperator


Sperator vs separator

chr(40) is the ( symbol. not a record separator/TOKEN. not sure if this is a good name for what your looking for

Technically, the correct separator would be the Unit Separator, chr(31), as that was intended to be the separator between fields (why “unit”? I don't know…) but practically it could be anything unlikely to otherwise occur in normal output so it isn't worth changing.

alan-stokes · 2018-05-29T09:31:45Z

spinn_utilities/make_tools/file_convertor.py

+        assert self._status == NORMAL_CODE
+        return self._process_line_normal_code(dest_f)
+
+    def _check_token(self):


This whole method isnt used anywhere that i can find (and wouldnt work for a function definition).

nuked dead code that was confusing Alan.

alan-stokes · 2018-05-29T09:40:32Z

spinn_utilities/make_tools/file_convertor.py

+            return self._process_line_in_log(dest_f)
+
+    def _process_line_normal_code(self, dest_f):
+        # Fast check


dont think you need the faster one. when your doing both and the second covers the first.

Nuked faster method as wasted more time remembering it then it will ever save

alan-stokes · 2018-05-29T10:11:27Z

spinn_utilities/make_tools/file_convertor.py

+
+        There is aslo the assumption that the start status is not COMMENT
+
+        :param dest_f:


fill in docs

alan-stokes · 2018-05-29T13:40:31Z

spinn_utilities/make_tools/file_convertor.py

+
+    def _write_log_method(self, dest_f, tail=""):
+        self._message_id += 1
+        self._log_full = self._log_full.replace('""', '')


missing in constructor self._log_full

missing self._log_lines in constructor

Added to slots but rather not add to init

alan-stokes · 2018-05-29T13:49:17Z

spinn_utilities/make_tools/file_converter.py

+            # Writing an extra newline here so need to recover that ASAP
+            self._too_many_lines += 1
+        end = tail + "\n"
+        if (self._log_lines <= 1):


redundant brackets

alan-stokes · 2018-05-29T13:56:18Z

spinn_utilities/make_tools/file_converter.py

+                self._line_num + 1,
+                original[1:-1]))
+
+    def _process_chars(self, dest_f):


this method is way too big, break up into status level processes.

alan-stokes · 2018-05-29T14:10:45Z

spinn_utilities/make_tools/replacer.py

+
+    def __init__(self, dict_pointer):
+        self._messages = {}
+        rest, extension = os.path.splitext(dict_pointer)


extension redundant, replace with _

alan-stokes · 2018-05-29T14:28:01Z

spinn_utilities/make_tools/replacer.py

+
+
+if __name__ == '__main__':
+    replacer = Replacer()


needs filling in, wont work currently. missing parameters

nuked broken main not needed

alan-stokes · 2018-05-29T14:28:16Z

spinn_utilities/make_tools/replacer.py

+            logger.error("Unable to find a dictionary file at {}"
+                         .format(dict_path))
+
+    def replace(self, short):


am yet to find whats calling this...... more a comment to chase this up

buffer extractor

changes requested done or disagreed with.

rowleya

Just a point of clarification...

rowleya · 2018-07-31T10:15:57Z

spinn_utilities/make_tools/file_converter.py

+                    self._log_full += (";")
+                    self._write_log_method(dest_f, line_num)
+                    pos = text.index(";") + 1
+                    write_flag == pos


Is this meant to be write_flag = pos?

…iNNUtils into convertor

fixed

Christian-B added 11 commits April 6, 2018 15:08

Revert "Revert "convert files before building""

dee5508

This reverts commit dc8fc5b.

improved convertyor and tests

2745146

merged in master

09fd825

fixed tests to work even if called higher

ed5ab1f

file to convert a whole directory

1d49b77

reduced print output

2b04530

Added replacer for going from dict codes back to text

e43a415

flake8

a7af2f0

Convertor always uses modified dir path to control dict range

3569509

merged in master

559e74a

changed token in log messages

515282c

Christian-B added the enhancement label Apr 26, 2018

Christian-B added this to the 5.0.0 milestone Apr 26, 2018

Christian-B assigned rowleya Apr 26, 2018

Christian-B added 4 commits April 26, 2018 16:44

fix for missing environ

6c8305f

removed unrequired octal

4cca122

handle the case of a \ after a log

e0be482

removed absolute dependency on an environment variable

4663a06

Christian-B and others added 4 commits May 1, 2018 09:38

dropped unrequired permission level

076c12a

merged in master

588f8ff

flake8

23deece

master into converter

e98a2af

alan-stokes suggested changes May 29, 2018

View reviewed changes

corrected spelling of converter

c1b3e09

alan-stokes suggested changes May 29, 2018

View reviewed changes

Christian-B added 2 commits May 29, 2018 15:20

minor style improvements

806e624

Added slots

2d928cf

alan-stokes previously requested changes May 29, 2018

View reviewed changes

Christian-B added 9 commits May 29, 2018 15:28

fixed FORMAT_EXP

9dd973b

nuked broken main

602c8d0

removed unused variable

a1d1b65

removed dead code

4960589

remove dead code

194b2bb

text and line_num changed to params as only changed at top level

663c960

removed _comment_start

617f81e

flake8

3cebd22

added more docs

f250ade

Christian-B and others added 3 commits June 6, 2018 10:19

added get_executable_paths

4a0b493

Merge branch 'master' into convertor

b83effd

Tidy up

057bb10

rowleya previously requested changes Jul 31, 2018

View reviewed changes

Christian-B and others added 3 commits July 31, 2018 11:28

fix incorrect assign

ada28aa

Fixed a blooper

1b275d7

Merge branch 'convertor' of https://github.com/SpiNNakerManchester/Sp…

9e06059

…iNNUtils into convertor

Ignore test file

1e454ad

rowleya merged commit eadf067 into master Jul 31, 2018

rowleya deleted the convertor branch July 31, 2018 10:36


		There is aslo the assumption that the start status is not COMMENT

		:param dest_f:

Dictionary for log messages #55

Dictionary for log messages #55

Conversation

Christian-B commented Apr 26, 2018

coveralls commented May 1, 2018 • edited

Pull Request Test Coverage Report for Build 566

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowleya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2018 •

edited