From b612ec9f67534818c655756fabece9fdfcb44baa Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Mon, 9 Nov 2015 20:52:43 +0100 Subject: [PATCH 01/10] Parsing hangs when file is inconsistent --- test/test_chordpro/invalid_content.sgc | 1 + test/test_chordpro/invalid_content.source | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 test/test_chordpro/invalid_content.sgc create mode 100644 test/test_chordpro/invalid_content.source diff --git a/test/test_chordpro/invalid_content.sgc b/test/test_chordpro/invalid_content.sgc new file mode 100644 index 00000000..768ee9ee --- /dev/null +++ b/test/test_chordpro/invalid_content.sgc @@ -0,0 +1 @@ +{lang: en} diff --git a/test/test_chordpro/invalid_content.source b/test/test_chordpro/invalid_content.source new file mode 100644 index 00000000..4dba23a7 --- /dev/null +++ b/test/test_chordpro/invalid_content.source @@ -0,0 +1,3 @@ +{soc} +Chorus +{eoc From 9dca42bc16293d2390add54b71be0d68330371a8 Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Mon, 9 Nov 2015 21:23:30 +0100 Subject: [PATCH 02/10] Problem localization? --- patacrep/songs/chordpro/syntax.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/patacrep/songs/chordpro/syntax.py b/patacrep/songs/chordpro/syntax.py index e88d7360..d765b79a 100644 --- a/patacrep/songs/chordpro/syntax.py +++ b/patacrep/songs/chordpro/syntax.py @@ -303,6 +303,10 @@ class ChordproParser(Parser): def p_error(self, token): super().p_error(token) + if not token: + # End of file + # Maybe it should raise an error ? + return token while True: token = self.parser.token() if not token or token.type == "ENDOFLINE": From db1a19a8733b8c7b9b32164ff16d85e958761f1b Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Tue, 10 Nov 2015 19:02:52 +0100 Subject: [PATCH 03/10] Only skip the erro if the end of the file wasn't reached --- patacrep/songs/chordpro/syntax.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/patacrep/songs/chordpro/syntax.py b/patacrep/songs/chordpro/syntax.py index d765b79a..37af7192 100644 --- a/patacrep/songs/chordpro/syntax.py +++ b/patacrep/songs/chordpro/syntax.py @@ -303,15 +303,12 @@ class ChordproParser(Parser): def p_error(self, token): super().p_error(token) - if not token: - # End of file - # Maybe it should raise an error ? - return token while True: token = self.parser.token() if not token or token.type == "ENDOFLINE": break - self.parser.errok() + if token: + self.parser.errok() return token def parse_song(content, filename=None): From 4cbbe29d28ac15d5d0220e5b61740ac4ac014f18 Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Tue, 10 Nov 2015 19:06:24 +0100 Subject: [PATCH 04/10] Localize were the error parsing should be made --- patacrep/songs/chordpro/syntax.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/patacrep/songs/chordpro/syntax.py b/patacrep/songs/chordpro/syntax.py index 37af7192..6c8d2540 100644 --- a/patacrep/songs/chordpro/syntax.py +++ b/patacrep/songs/chordpro/syntax.py @@ -313,7 +313,13 @@ class ChordproParser(Parser): def parse_song(content, filename=None): """Parse song and return its metadata.""" - return ChordproParser(filename).parse( + parser = ChordproParser(filename) + parsed_content = parser.parse( content, lexer=ChordProLexer(filename=filename).lexer, ) + if parsed_content is None: + # There was a fatal error parsing the content + # TODO: implement error handling by looking into parser + raise Exception('The song could not be parsed') + return parsed_content From f70df32d6dd68dca4882c1718ed3b4447b5a7089 Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Thu, 12 Nov 2015 11:16:41 +0100 Subject: [PATCH 05/10] Use files.chdir --- test/test_chordpro/test_parser.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index 6834af4b..692eaa28 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -47,10 +47,8 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): def chdir(): """Context to temporarry change current directory to this file directory """ - olddir = os.getcwd() - os.chdir(resource_filename(__name__, "")) - yield - os.chdir(olddir) + with files.chdir(resource_filename(__name__, "")): + yield def assertRender(self, base, destformat): # pylint: disable=invalid-name """Assert that `{base}.source` is correctly rendered in the `destformat`. From 90c77201a9a20c0aadc54aff43c3d3c1fa58021e Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Thu, 12 Nov 2015 11:23:10 +0100 Subject: [PATCH 06/10] Useless existence test --- test/test_chordpro/test_parser.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index 692eaa28..4315e241 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -98,8 +98,6 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): def test_parse_render(self): """Test that `base` is correctly parsed and rendered.""" - if base is None or dest is None: - return self.assertRender(base, dest) test_parse_render.__doc__ = ( From 5eae2230b2a0aec212f546a9c4dc4351401755fc Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Thu, 12 Nov 2015 11:30:44 +0100 Subject: [PATCH 07/10] Implement test for parsing failures --- patacrep/songs/chordpro/syntax.py | 3 +-- .../{ => errors}/invalid_content.source | 0 test/test_chordpro/invalid_content.sgc | 1 - test/test_chordpro/test_parser.py | 26 +++++++++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) rename test/test_chordpro/{ => errors}/invalid_content.source (100%) delete mode 100644 test/test_chordpro/invalid_content.sgc diff --git a/patacrep/songs/chordpro/syntax.py b/patacrep/songs/chordpro/syntax.py index 6c8d2540..6379ca62 100644 --- a/patacrep/songs/chordpro/syntax.py +++ b/patacrep/songs/chordpro/syntax.py @@ -320,6 +320,5 @@ def parse_song(content, filename=None): ) if parsed_content is None: # There was a fatal error parsing the content - # TODO: implement error handling by looking into parser - raise Exception('The song could not be parsed') + raise SyntaxError('Fatal error during song parsing: {}'.format(filename)) return parsed_content diff --git a/test/test_chordpro/invalid_content.source b/test/test_chordpro/errors/invalid_content.source similarity index 100% rename from test/test_chordpro/invalid_content.source rename to test/test_chordpro/errors/invalid_content.source diff --git a/test/test_chordpro/invalid_content.sgc b/test/test_chordpro/invalid_content.sgc deleted file mode 100644 index 768ee9ee..00000000 --- a/test/test_chordpro/invalid_content.sgc +++ /dev/null @@ -1 +0,0 @@ -{lang: en} diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index 4315e241..d14bcb4d 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -91,6 +91,14 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): cls._create_test(base, dest), ) + with files.chdir('errors'): + for source in sorted(glob.glob('*.source')): + base = source[:-len(".source")] + yield ( + "test_{}_failure".format(base), + cls._create_failure(base), + ) + @classmethod def _create_test(cls, base, dest): """Return a function testing that `base` compilation in `dest` format. @@ -105,6 +113,24 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): ).format(base=os.path.basename(base), format=dest) return test_parse_render + @classmethod + def _create_failure(cls, base): + """Return a function testing that `base` fails. + """ + + def test_parse_render(self): + """Test that `base` parsing fails.""" + sourcename = "{}.source".format(base) + with self.chdir(): + with files.chdir('errors'): + parser = self.song_plugins[LANGUAGES['sgc']]['sgc'] + self.assertRaises(SyntaxError, parser, sourcename, self.config) + + test_parse_render.__doc__ = ( + "Test that '{base}' parsing fails." + ).format(base=os.path.basename(base)) + return test_parse_render + @classmethod def _overwrite_clrf(cls): """Overwrite `*.crlf.source` files to force the CRLF line endings. From 46ab0b047f372219af74acc9e91e7604eff7c393 Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Thu, 12 Nov 2015 11:39:50 +0100 Subject: [PATCH 08/10] Code cleaning --- patacrep/songs/chordpro/syntax.py | 1 - test/test_chordpro/test_parser.py | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/patacrep/songs/chordpro/syntax.py b/patacrep/songs/chordpro/syntax.py index 6379ca62..a1c69983 100644 --- a/patacrep/songs/chordpro/syntax.py +++ b/patacrep/songs/chordpro/syntax.py @@ -319,6 +319,5 @@ def parse_song(content, filename=None): lexer=ChordProLexer(filename=filename).lexer, ) if parsed_content is None: - # There was a fatal error parsing the content raise SyntaxError('Fatal error during song parsing: {}'.format(filename)) return parsed_content diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index d14bcb4d..f369748c 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -103,11 +103,7 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): def _create_test(cls, base, dest): """Return a function testing that `base` compilation in `dest` format. """ - - def test_parse_render(self): - """Test that `base` is correctly parsed and rendered.""" - self.assertRender(base, dest) - + test_parse_render = lambda self: self.assertRender(base, dest) test_parse_render.__doc__ = ( "Test that '{base}' is correctly parsed and rendererd into '{format}' format." ).format(base=os.path.basename(base), format=dest) @@ -115,7 +111,7 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): @classmethod def _create_failure(cls, base): - """Return a function testing that `base` fails. + """Return a function testing that `base` parsing fails. """ def test_parse_render(self): From 1f09d6af780f9db39beb5dddb40aaf70679c1f6e Mon Sep 17 00:00:00 2001 From: Oliverpool Date: Thu, 12 Nov 2015 11:51:33 +0100 Subject: [PATCH 09/10] Rename parse_failure test function --- test/test_chordpro/test_parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index f369748c..6de149ac 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -114,7 +114,7 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): """Return a function testing that `base` parsing fails. """ - def test_parse_render(self): + def test_parse_failure(self): """Test that `base` parsing fails.""" sourcename = "{}.source".format(base) with self.chdir(): @@ -122,10 +122,10 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): parser = self.song_plugins[LANGUAGES['sgc']]['sgc'] self.assertRaises(SyntaxError, parser, sourcename, self.config) - test_parse_render.__doc__ = ( + test_parse_failure.__doc__ = ( "Test that '{base}' parsing fails." ).format(base=os.path.basename(base)) - return test_parse_render + return test_parse_failure @classmethod def _overwrite_clrf(cls): From b1eb5a0f879f73c6294672961645eed71fd41b06 Mon Sep 17 00:00:00 2001 From: Louis Date: Thu, 12 Nov 2015 21:02:03 +0100 Subject: [PATCH 10/10] Use variable number of arguments for `chdir` --- patacrep/files.py | 6 +++--- test/test_chordpro/test_parser.py | 15 +++++++-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/patacrep/files.py b/patacrep/files.py index a204f8e7..08d7b473 100644 --- a/patacrep/files.py +++ b/patacrep/files.py @@ -61,17 +61,17 @@ def path2posix(string): ) @contextmanager -def chdir(path): +def chdir(*path): """Locally change dir Can be used as: - with chdir("some/directory"): + with chdir("some", "directory"): do_stuff() """ olddir = os.getcwd() if path: - os.chdir(path) + os.chdir(os.path.join(*path)) yield os.chdir(olddir) else: diff --git a/test/test_chordpro/test_parser.py b/test/test_chordpro/test_parser.py index 6de149ac..fe6dc7c5 100644 --- a/test/test_chordpro/test_parser.py +++ b/test/test_chordpro/test_parser.py @@ -44,10 +44,10 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): @staticmethod @contextlib.contextmanager - def chdir(): - """Context to temporarry change current directory to this file directory + def chdir(*path): + """Context to temporarry change current directory, relative to this file directory """ - with files.chdir(resource_filename(__name__, "")): + with files.chdir(resource_filename(__name__, ""), *path): yield def assertRender(self, base, destformat): # pylint: disable=invalid-name @@ -91,7 +91,7 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): cls._create_test(base, dest), ) - with files.chdir('errors'): + with cls.chdir('errors'): for source in sorted(glob.glob('*.source')): base = source[:-len(".source")] yield ( @@ -117,10 +117,9 @@ class FileTest(unittest.TestCase, metaclass=dynamic.DynamicTest): def test_parse_failure(self): """Test that `base` parsing fails.""" sourcename = "{}.source".format(base) - with self.chdir(): - with files.chdir('errors'): - parser = self.song_plugins[LANGUAGES['sgc']]['sgc'] - self.assertRaises(SyntaxError, parser, sourcename, self.config) + with self.chdir('errors'): + parser = self.song_plugins[LANGUAGES['sgc']]['sgc'] + self.assertRaises(SyntaxError, parser, sourcename, self.config) test_parse_failure.__doc__ = ( "Test that '{base}' parsing fails."