Add context manager/decorator to set context gfyear (+ misc. cleanup) #2

Mortal · 2016-12-22T15:59:37Z

Omdøb prefix, postfix, kprefix funktioner
Erstat "" + ... med assert isinstance(..., str)
Tilføj get_gfyear funktion til at oversætte gfyear=None til nuværende context gfyear
Tilføj override context manager + decorator til at ændre context gfyear
Implementér parse

coveralls · 2016-12-22T16:01:09Z

Coverage increased (+3.3%) to 75.748% when pulling 544eb41 on pull_2 into 95701f4 on master.

coveralls · 2016-12-22T16:01:09Z

Coverage increased (+3.3%) to 75.748% when pulling 544eb41 on pull_2 into 95701f4 on master.

coveralls · 2016-12-22T16:41:10Z

Coverage increased (+7.7%) to 80.151% when pulling 1de4e0f on pull_2 into 95701f4 on master.

coveralls · 2016-12-22T16:41:10Z

Coverage increased (+7.7%) to 80.151% when pulling 1de4e0f on pull_2 into 95701f4 on master.

neic

Jeg havde forestillet mig et lille offentligt API af kun prefix(), kprefix(), postfix(), parse(), set_gfyear(), måske get_gfyear() samt nogle string literals til type. Hvis de er kodet rigtigt tror jeg at de er kraftige nok til at løse de fleste problemer vi støder på.

neic · 2016-12-22T23:30:28Z

tktitler.py

-    _validate(titletupel, gfyear)
+def get_gfyear(argument_gfyear=None):
+    if argument_gfyear is None:
+        r = gfyear


Det var ikke meningen at gfyear skulle være defineret nogen steder i tktitler. gfyear=2016 i linje ca. 9 var bare WIP til uformelle test. Jeg mener at enten skal man give funktionerne gfyear som argument, eller sætte med override(). I django vil man typisk sætte override(config.GFYEAR).

Jeg har ændret det så der kommer en ValueError hvis man ikke har sat et context gfyear.

neic · 2016-12-22T23:30:41Z

tktitler.py

@@ -42,14 +83,14 @@ def unicode_superscript(n):
        return 'T%sO%s' % (sup_fn(age - 3), root)


-def kprefix(titletupel, gfyear=gfyear, type=PREFIXTYPE_NORMAL):
+def ktk_prefix(titletupel, gfyear=gfyear, type=PREFIXTYPE_NORMAL):


Burde den ikke hedde tk_kprefix?

neic · 2016-12-22T23:33:19Z

tktitler.py

+    return re.sub(r'[^0-9A-ZÆØÅ]', lambda mo: tr(mo.group(0)), s)
+
+
+def parse_prefix(prefix):


Skal parse_prefix, parse_postfix, parse_relative (og normalize?) være offentlige? parse skulle kunne tage sig af det hele og så er APIet vi skal vedligeholde mindre.

Jeg synes godt vi kan have en offentlig parse_relative funktion da der ellers ikke er nogen nem måde at... parse relative titler på. Jeg er enig i at resten ikke bør være offentlige.

neic · 2016-12-22T23:35:49Z

tktitler.py

@@ -158,7 +277,7 @@ def _multireplace(string, replacements):
    return regexp.sub(lambda match: replacements[match.group(0)], string)


-def get_period(prefix, postfix, gfyear):
+def get_period(prefix, postfix, gfyear=None):


get_periodog parse_bestfu_alias havde jeg tænkt helt skulle erstattes helt af parse. De er en del af filen af historisk interesse.

Det er fint med de nye get_period(), men det viser vel at parse() nemt erstatte den. Kan det ikke være op til brugeren af biblioteket at skrive den ene linje hvis det er netop period de skal bruge?

Ja, lad os fjerne get_period og parse_bestfu_alias.

neic · 2016-12-22T23:37:44Z

tktitler.py

+        return wrapped
+
+
+def override(gfyear):


Er override det rigtige navn? Jeg havde forestillet mig noget i stil med set_gfyear.

Ulempen ved navnet set_gfyear er at man kunne forvente at man kan sætte et gfyear ved blot at kalde set_gfyear. Det virker mærkeligt at følgende ikke duer:

set_gfyear(2013) print(parse('GFUHI'))

Men set_gfyear er nok stadig et bedre navn end override.

…hanged

coveralls · 2016-12-23T21:16:32Z

Coverage increased (+18.04%) to 90.476% when pulling 1b19a5a on pull_2 into 95701f4 on master.

coveralls · 2016-12-23T21:19:57Z

Coverage increased (+18.04%) to 90.476% when pulling 940871f on pull_2 into 95701f4 on master.

coveralls · 2016-12-23T21:19:57Z

Coverage increased (+18.04%) to 90.476% when pulling 940871f on pull_2 into 95701f4 on master.

coveralls · 2016-12-23T23:45:54Z

Coverage increased (+18.09%) to 90.529% when pulling 9d9e775 on pull_2 into 95701f4 on master.

neic

Der er efterhånden mere arbejde i det her PR, end master. Jeg merger, så kan vi arbejde videre på det i master eller lave et nyt PR.

neic · 2016-12-24T01:45:35Z

tktitler.py

@@ -158,7 +277,7 @@ def _multireplace(string, replacements):
    return regexp.sub(lambda match: replacements[match.group(0)], string)


-def get_period(prefix, postfix, gfyear):
+def get_period(prefix, postfix, gfyear=None):


Det er fint med de nye get_period(), men det viser vel at parse() nemt erstatte den. Kan det ikke være op til brugeren af biblioteket at skrive den ene linje hvis det er netop period de skal bruge?

neic · 2016-12-24T01:50:07Z

tktitler.py

-
-
-def parse_bestfu_alias(alias, gfyear):
+def parse_bestfu_alias(alias, gfyear=None):


Er parse_bestfu_alias() ikke meget specifik til mailsystements problemer? Vil en mere generelt løsning ikke være en funktion der tager et alias, root eller title og returnerer en kind?

Jo, lad os fjerne get_period og parse_bestfu_alias. Måske skal tktitler have en funktion der kan fortælle om noget er en BEST-titel eller FU-titel, men det er ikke sikkert. (Vi kan også godt forsvare at simplificere mailsystemet og fjerne dets behov for at klassificere om en titel er BEST eller FU.)

neic · 2016-12-24T01:51:51Z

tktitler.py



-def _validate(titletupel, gfyear):
-    root, period = titletupel
+def parse_relative(input_alias):


Hvad er brugsscenariet for parse_relative() uden for tktitler?

Det virker som en naturlig subrutine for parse, og det kan blive nyttigt hvis vi f.eks. vil lave en mere intelligent håndtering af EFUIT-titler (hvor det f.eks. kan gælde at GEFUIT14 er den samme som EFUIT12).

Funktionen parse er nok den mest anvendelige, men parse_relative er en lidt mindre lossy parsing af aliaset. Man kunne forestille sig at lave en lossless parser (så enhver streng der repræsenterer en titel, såsom "K2TOGFUET1516", parses til en struktur der indeholder nok information til at rekonstruere den præcis samme streng), og ud fra denne vil parse_relative være en simpel nedkogning af parserens output, og parse vil ligeledes (som nu) være en nedkogning af parse_relative.

Diskutionen er flyttet til #6.

Mortal added 11 commits December 22, 2016 16:29

Rename {pre,post}fix functions to tk_{pre,post}fix

65cd528

Rename kprefix function to ktk_prefix

629e4b0

Update tests

9e90fed

Rename prefix_/postfix_ variables

829287a

Add type assertions

f43c1cf

Remove superfluous string concatenation

2451c59

Add _Override and override function

833512b

Introduce get_gfyear and make _validate return gfyear

d3c9105

Use get_gfyear

785631e

Use get_gfyear in parse, get_period, parse_bestfu_alias

9609bb9

Add override test

544eb41

Mortal assigned neic Dec 22, 2016

Mortal added 2 commits December 22, 2016 17:37

Introduce parse_relative with tests

e87080f

Add parse() tests

1de4e0f

neic reviewed Dec 22, 2016

View reviewed changes

Mortal added 11 commits December 23, 2016 21:50

tk_kt

2baaa65

tk_kt test

c36588a

Make gfyear variable non-public

cd75079

Rename override -> set_gfyear

e232d51

Make normalize non-public

f0a9e59

Make parse_postfix non-public

a89d9cb

Make parse_prefix non-public

9671ee2

Set _gfyear to a special nil value and raise a helpful error if not c…

c60c25d

…hanged

Add blank spaces to doctest

8800028

Fix doctest

238dbfa

Fix tk_kprefix

83e528a

Mortal added 5 commits December 23, 2016 22:07

Implement get_period and parse_bestfu_alias by simply deferring to parse

85ce079

Refactor, introducing _validate_title

91f01cf

Remove gfyear argument from tk_postfix

d563d11

Fix variable name

45ee75e

Fix test

1b19a5a

Rename variable titletupel -> title

940871f

Mortal added 3 commits December 24, 2016 00:44

Refactor tk_prefix/tk_kprefix slightly

32e6b2c

Fix parse_relative

dcc2eba

Add tests for parsing prefix/postfix without a root

9d9e775

neic approved these changes Dec 24, 2016

View reviewed changes

neic merged commit 44bf78d into master Dec 24, 2016

neic mentioned this pull request Dec 24, 2016

public parse_relative() #6

Closed

neic deleted the pull_2 branch January 15, 2017 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context manager/decorator to set context gfyear (+ misc. cleanup) #2

Add context manager/decorator to set context gfyear (+ misc. cleanup) #2

Mortal commented Dec 22, 2016 •

edited

Loading

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

neic left a comment

neic Dec 22, 2016 •

edited

Loading

Mortal Dec 23, 2016

neic Dec 22, 2016

Mortal Dec 23, 2016 •

edited

Loading

neic Dec 22, 2016

Mortal Dec 23, 2016

neic Dec 22, 2016

neic Dec 24, 2016

Mortal Dec 24, 2016

neic Dec 22, 2016

Mortal Dec 23, 2016 •

edited

Loading

coveralls commented Dec 23, 2016 •

edited

Loading

coveralls commented Dec 23, 2016

coveralls commented Dec 23, 2016

coveralls commented Dec 23, 2016 •

edited

Loading

neic left a comment

neic Dec 24, 2016

neic Dec 24, 2016

Mortal Dec 24, 2016

neic Dec 24, 2016

neic Dec 24, 2016

Mortal Dec 24, 2016

neic Dec 24, 2016

		return re.sub(r'[^0-9A-ZÆØÅ]', lambda mo: tr(mo.group(0)), s)


		def parse_prefix(prefix):



		def parse_bestfu_alias(alias, gfyear):
		def parse_bestfu_alias(alias, gfyear=None):

Add context manager/decorator to set context gfyear (+ misc. cleanup) #2

Add context manager/decorator to set context gfyear (+ misc. cleanup) #2

Conversation

Mortal commented Dec 22, 2016 • edited Loading

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

neic left a comment

Choose a reason for hiding this comment

neic Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mortal Dec 23, 2016 • edited Loading

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

Mortal Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Dec 23, 2016 • edited Loading

coveralls commented Dec 23, 2016

coveralls commented Dec 23, 2016

coveralls commented Dec 23, 2016 • edited Loading

neic left a 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

Mortal commented Dec 22, 2016 •

edited

Loading

neic Dec 22, 2016 •

edited

Loading

Mortal Dec 23, 2016 •

edited

Loading

Mortal Dec 23, 2016 •

edited

Loading

coveralls commented Dec 23, 2016 •

edited

Loading

coveralls commented Dec 23, 2016 •

edited

Loading