generation of Latex code and replacement in template #60

DonMischo · 2022-01-07T13:33:34Z

no tests so far shame on me. had to sort out how to do it. would've been easier having the Latex template first.
Some warnings for string.concat and string.replace

no tests so far *shame on me*. had to sort out how to do it. would've been easier having the Latex template first. Some warnings for string.concat and string.replace

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java

src/test/resources/placeholders/replace.tex

ArchibaldBienetre · 2022-01-07T15:35:39Z

no tests so far shame on me

🤷 imo, this just makes it harder for you to catch edge cases and understand if anything is still missing

Usually, you could

create a so-called "spike" to try things out, then throw that away
try things out in a learning test

as an alternative

ArchibaldBienetre · 2022-01-07T15:35:58Z

❌ Some tests are failing on CI, could you have a look?

It seems to be a null issue

Caused by: java.lang.NullPointerException
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.placeholders.NoopPlaceholderReplacer.replaceBaseData(NoopPlaceholderReplacer.java:67)
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.placeholders.NoopPlaceholderReplacer.replacePlaceholdersInTexFile(NoopPlaceholderReplacer.java:24)
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.rendering.SchoolReportsRenderer.renderSingleSchoolReportPdf(SchoolReportsRenderer.java:81)
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.rendering.SchoolReportsRenderer.renderStudentReports(SchoolReportsRenderer.java:74)
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.rendering.SchoolReportsRenderer.renderSchoolReportsForGivenFiles(SchoolReportsRenderer.java:65)
	at org.example.csv2tex@1.0-SNAPSHOT/org.example.csv2tex.rendering.SchoolReportsRenderer.renderSchoolReportsForGivenFiles(SchoolReportsRenderer.java:54)
	... 84 more

EDIT: ✔️ Fixed, mixup of file and file content string

ArchibaldBienetre · 2022-01-07T15:36:53Z

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java

+    public String replaceBaseData(String texFileContent, SchoolReportData schoolReportData) {
+        texFileContent.replace("#givenName", schoolReportData.givenName);
+        texFileContent.replace("#surName", schoolReportData.surName);
+        texFileContent.replace("#birthDay", schoolReportData.birthDay);
+        texFileContent.replace("#schoolClass", schoolReportData.schoolClass);
+        texFileContent.replace("#schoolYear", schoolReportData.schoolYear);
+        texFileContent.replace("#partOfYear", schoolReportData.partOfYear);
+
+        return texFileContent;
+    }


Nice method scope, nice encapsulation... as a "teacher", I'm a bit proud here :) 👍

(well, only the public modifier should be changed, but that should be easy enough)

no tests so far *shame on me*. had to sort out how to do it. would've been easier having the Latex template first. Some warnings for string.concat and string.replace

ArchibaldBienetre · 2022-01-09T12:29:46Z

@DonMischo to have a cleaner commit history, I rebased the branch onto main

…lacements

no tests so far *shame on me*. added Tex template adapted code for tex generation

100% test coverage added some options for level and grades

ArchibaldBienetre · 2022-01-21T16:05:44Z

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java

+                firstSchoolcompetencyData.schoolSubject.equals("Mathematik") ||
+                firstSchoolcompetencyData.schoolSubject.equals("Deutsch") ||
+                firstSchoolcompetencyData.schoolSubject.equals("Englisch")) {


Hard-coded German subject names? :(

Currently, it's a necessity

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java

ArchibaldBienetre · 2022-01-21T16:35:25Z

src/test/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacerTest.java

+        assertThat(level).isEqualTo("rot");
+        assertThat(level1).isEqualTo("rot");
+        assertThat(level2).isEqualTo("blau");
+        assertThat(level3).isEqualTo("grün");


Why German? 🤔
Is it part of a package?

German names of the subject levels - we could add an indirection for the future, though

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java

nice trap I set for Micha m(

…lacements

ArchibaldBienetre · 2022-01-21T17:32:01Z

src/main/resources/ZeugnisTemplateHJ.tex

@@ -0,0 +1,216 @@
+\documentclass[11pt,a4paper]{article}


Good work, looks great !! :)

What I may want to ask

that we rename this file SchoolReportTemplateHalfYear_HalbjahreszeugnisTemplat.tex

that we rename some things that use an abbreviation, currently (e.g. SS -> minorSubject, MS -> mainSubject)

that we move this file somewhere else (not production code resources), e.g. a dedicated folder at root level of the repo

that we can get rid of German names like "grün" in Java - and use an indirection here (? sry, not my expertise ':D)

~~that we add a simple integration test that runs this template, and checks that this does not fail (DAU proofing for future local changes)~~ EDIT: No, should be part of days/hours absent #58, as it fails currently with as-of-yet-unsupported placeholders

I let "grün" etc in there. It makes more sense to move it to a "school-specific extension" in the future if there is demand.

⬆️ Moved to issue #64

put commands in extra tex file put packages in extra tex file

removed comments below

Let's keep this in English, and spell out any assumed abbreviations. The code should be easily understood - we go for professional code here :)

Like before - a variable's role should be easy to understand. Replacements for these will be introduced by #58

It's not a template, all placeholders are replaced already.

as usual, good naming is key

ArchibaldBienetre · 2022-01-23T15:40:02Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+        return texFileContent;
+    }
+
+    @VisibleForTesting


@DonMischo usually, having to make methods non-private for tests is a sign of bad encapsulation.

As it is unexpected that purely-internal code is not public, it's good style to mark it (e.g. by adding @VisibleForTesting )

ArchibaldBienetre · 2022-01-23T15:40:15Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+        return subjectTable.toString();
+    }
+
+    // This is a HACK - Michael needed to introduce it in hard-coded form for now


We should be honest about such things!

ArchibaldBienetre · 2022-01-23T15:40:41Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+
+    // This is a HACK - Michael needed to introduce it in hard-coded form for now
+    private boolean shouldRenderAsMajorSubject(SchoolCompetencyData firstSchoolCompetencyData, String partOfYear) {
+        return partOfYear.equals("Endjahr") ||


@DonMischo Why is it an "OR" with end of year?

Because all subjects are graded at the end of the year??

ArchibaldBienetre · 2022-01-23T15:43:22Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+        switch (grade) {
+            case "1":
+                return "\\gradeOne";
+            case "2":
+                return "\\gradeTwo";
+            case "3":
+                return "\\gradeThree";
+            case "4":
+                return "\\gradeFour";
+            case "hj":
+                return "\\gradeHalfYear";
+            case "nb":
+                return "\\gradeNotGiven";


@DonMischo this is something that we should have talked about - you introduce special values to the data format that we didn't talk about nor test before.

"Stringly-typed", as this is called, can work, but once more, such things should be made really explicit.

At the very least, we should document it in the code of org.example.csv2tex.data.SchoolReportData

ArchibaldBienetre · 2022-01-23T15:45:51Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+            case "hj":
+                return "\\gradeHalfYear";
+            case "nb":
+                return "\\gradeNotGiven";


@DonMischo Does it mean the subject is ungraded in general? If so, let's make the naming more clear here.

ArchibaldBienetre · 2022-01-23T15:47:17Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+            case "1":
+                return "\\gradeOne";
+            case "2":
+                return "\\gradeTwo";
+            case "3":
+                return "\\gradeThree";
+            case "4":
+                return "\\gradeFour";


@DonMischo I really don't like that our so-far very generic all-purpose software is now super specific about what strings it expects :(

But it will do for now, we could exctract anything specific to Erfurt to a separate plugin later.

ArchibaldBienetre · 2022-01-23T15:51:17Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+    @VisibleForTesting
+    String replaceBaseData(String texFileContent, SchoolReportData schoolReportData) {
+        texFileContent = texFileContent
+                .replace(TEX_TEMPLATE_PLACEHOLDER_GIVEN_NAME, schoolReportData.givenName)
+                .replace(TEX_TEMPLATE_PLACEHOLDER_SURNAME, schoolReportData.surName)
+                .replace(TEX_TEMPLATE_PLACEHOLDER_BIRTHDAY, schoolReportData.birthDay)
+                .replace(TEX_TEMPLATE_PLACEHOLDER_SCHOOL_CLASS, schoolReportData.schoolClass)
+                .replace(TEX_TEMPLATE_PLACEHOLDER_SCHOOL_YEAR, schoolReportData.schoolYear)
+                .replace(TEX_TEMPLATE_PLACEHOLDER_PART_OF_YEAR, schoolReportData.partOfYear);
+        return texFileContent;
+    }


@DonMischo I desperately tried to address in numerous places that we should match for word boundaries here ... but screw it, let's leave it for now

ArchibaldBienetre · 2022-01-23T15:52:59Z

src/test/java/org/example/csv2tex/placeholders/PlaceholderReplacerImplTest.java

+        String grade = sut.makeGrade(schoolReportData.schoolCompetencies.get(0).grade);
+        String grade1 = sut.makeGrade("1");
+        String grade2 = sut.makeGrade("2");
+        String grade3 = sut.makeGrade("3");
+        String grade4 = sut.makeGrade("4");
+        String gradeNotGiven = sut.makeGrade("nb");
+        String gradeHalfYear = sut.makeGrade("hj");
+        String gradeFalse = sut.makeGrade("7");


Parameterized test would be a better fit, but I don't want to rewrite everything again...

ArchibaldBienetre · 2022-01-23T15:55:53Z

src/test/java/org/example/csv2tex/placeholders/PlaceholderReplacerImplTest.java

+    @Test
+    public void makeGrade() {
+        SchoolReportData schoolReportData = generateSchoolReportData();
+        String grade = sut.makeGrade(schoolReportData.schoolCompetencies.get(0).grade);
+        String grade1 = sut.makeGrade("1");
+        String grade2 = sut.makeGrade("2");
+        String grade3 = sut.makeGrade("3");
+        String grade4 = sut.makeGrade("4");
+        String gradeNotGiven = sut.makeGrade("nb");
+        String gradeComesWithSecondHalfYear = sut.makeGrade("hj");
+        String gradeFalse = sut.makeGrade("7");
+
+        assertThat(grade).isEqualTo("\\gradeOne");
+        assertThat(grade1).isEqualTo("\\gradeOne");
+        assertThat(grade2).isEqualTo("\\gradeTwo");
+        assertThat(grade3).isEqualTo("\\gradeThree");
+        assertThat(grade4).isEqualTo("\\gradeFour");
+        assertThat(gradeNotGiven).isEqualTo("\\gradeNotGiven");
+        assertThat(gradeComesWithSecondHalfYear).isEqualTo("\\gradeComesWithSecondHalfYear");
+        assertThat(gradeFalse).isEqualTo("");
+    }


such stuff should be a parameterized test, but it's okay 🤷

ArchibaldBienetre · 2022-01-23T16:22:15Z

src/main/java/org/example/csv2tex/placeholders/PlaceholderReplacerImpl.java

+            if (!currentSubject.equals(schoolCompetencyData.schoolSubject)) {
+                currentSubject = schoolCompetencyData.schoolSubject;
+
+                tables.append(makeTableEntry(competencyList, partOfYear));
+                //FIXME
+                competencyList.removeAll(competencyList);
+                competencyList.add(schoolCompetencyData);


Reason for the confusing removeAll:

Generates a new table part for each new subject

It's a bit of a weird side effect here, I'll leave it mostly intact, though, and make the code easier to read

this bit was confusing, I'm not sure it works as-is!

makes it way easier to grasp what is going on, imo

ArchibaldBienetre

This bit is still a bit mysterious: #60 (comment)

Apart from that, I guess the code is quite okay now :)

fix: default grade row command

ArchibaldBienetre · 2022-01-25T08:00:42Z

src/test/java/org/example/csv2tex/placeholders/PlaceholderReplacerImplTest.java

+        assertThat(grade4).isEqualTo("\\gradeFour");
+        assertThat(gradeNotGiven).isEqualTo("\\gradeNotGiven");
+        assertThat(gradeComesWithSecondHalfYear).isEqualTo("\\gradeComesWithSecondHalfYear");
+        assertThat(gradeFalse).isEqualTo("\\gradeDefault");


@DonMischo this is why the test was failing, because you changed production code without adjusting the test case

@DonMischo

I don't know if this will work, @DonMischo must test

ArchibaldBienetre · 2022-01-25T08:07:32Z

src/test/java/org/example/csv2tex/rendering/SchoolReportsRendererTest.java

@@ -48,7 +48,7 @@ public void renderSchoolReportsForGivenFiles_withNonExistentCsv_throwsException(
        assertThatThrownBy(() -> sut.renderSchoolReportsForGivenFiles(NONEXISTENT_CSV_FILE, NO_PLACEHOLDERS_TEX_FILE))
                .describedAs("As the UI prevents the user from selecting a nonexistent file, we expect a generic exception")
                .isInstanceOf(RenderingException.class)
-                .hasMessageContaining("No such file")
+                .hasMessageMatching(".*(No such file|nicht gefunden).*")


@DonMischo please check if this fixes tests no your German Ubuntu

ArchibaldBienetre · 2022-01-26T08:39:40Z

@DonMischo good for merge? :)

The rest (new placeholders...), that is a separate issue and should be a separate pull request

ArchibaldBienetre · 2022-01-30T19:15:05Z

No response on any communication channel. I'm merging this then.

generation of Latex code and replacement in template

c1f2403

no tests so far *shame on me*. had to sort out how to do it. would've been easier having the Latex template first. Some warnings for string.concat and string.replace

ArchibaldBienetre reviewed Jan 7, 2022

View reviewed changes

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java Outdated Show resolved Hide resolved

ArchibaldBienetre reviewed Jan 7, 2022

View reviewed changes

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java Outdated Show resolved Hide resolved

ArchibaldBienetre reviewed Jan 7, 2022

View reviewed changes

src/test/resources/placeholders/replace.tex Outdated Show resolved Hide resolved

ArchibaldBienetre reviewed Jan 7, 2022

View reviewed changes

generation of Latex code and replacement in template

737579a

no tests so far *shame on me*. had to sort out how to do it. would've been easier having the Latex template first. Some warnings for string.concat and string.replace

ArchibaldBienetre force-pushed the replacements branch from c1f2403 to 737579a Compare January 9, 2022 12:29

DonMischo added 5 commits January 12, 2022 09:21

Merge branch 'replacements' of github.com:SergelsOrg/csv2tex into rep…

81f0b5e

…lacements

generation of Latex code and replacement in template

e3a3e88

no tests so far *shame on me*. added Tex template adapted code for tex generation

first tests for tex replacement

df86b49

tests for replacement

336d36d

generation of Latex code and replacement in template

a5a3c00

100% test coverage added some options for level and grades

ArchibaldBienetre reviewed Jan 21, 2022

View reviewed changes

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java Outdated Show resolved Hide resolved

ArchibaldBienetre reviewed Jan 21, 2022

View reviewed changes

fix: texTemplate already given as string

701a36c

ArchibaldBienetre reviewed Jan 21, 2022

View reviewed changes

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java Outdated Show resolved Hide resolved

ArchibaldBienetre reviewed Jan 21, 2022

View reviewed changes

src/main/java/org/example/csv2tex/placeholders/NoopPlaceholderReplacer.java Outdated Show resolved Hide resolved

DonMischo and others added 4 commits January 21, 2022 17:50

fix: IDE warnings

b0b0bd2

adjust confusing naming

b14a38a

nice trap I set for Micha m(

tex file removed some comments

f3042be

Merge branch 'replacements' of github.com:SergelsOrg/csv2tex into rep…

bf118f6

…lacements

ArchibaldBienetre reviewed Jan 21, 2022

View reviewed changes

tex template

65cf3ef

put commands in extra tex file put packages in extra tex file

ArchibaldBienetre mentioned this pull request Jan 21, 2022

TeX template for real school reports #49

Closed

tex template

036b2b6

removed comments below

ArchibaldBienetre mentioned this pull request Jan 21, 2022

days/hours absent #58

Closed

1 task

ArchibaldBienetre added 8 commits January 23, 2022 16:04

rename some tex functions

76b3ce7

Let's keep this in English, and spell out any assumed abbreviations. The code should be easily understood - we go for professional code here :)

rename placeholders

21d86a3

Like before - a variable's role should be easy to understand. Replacements for these will be introduced by #58

rename file

c731b8e

It's not a template, all placeholders are replaced already.

remove outdated comment

f51aa81

adjust method visibilities; extract constants

6951222

rename tex function

b31c5f9

as usual, good naming is key

extract method, mark hack

7c1b97e

further constants extracted, renamed things for clarity

947afe9

ArchibaldBienetre reviewed Jan 23, 2022

View reviewed changes

ArchibaldBienetre added 2 commits January 23, 2022 17:25

refactor replacer code

3cf2cb3

this bit was confusing, I'm not sure it works as-is!

extract methods

7aff3d6

makes it way easier to grasp what is going on, imo

ArchibaldBienetre force-pushed the replacements branch from 200a60f to 7aff3d6 Compare January 23, 2022 16:31

ArchibaldBienetre added 3 commits January 23, 2022 17:34

TDD: add failing test for hidden bug

b665b91

fix hidden bug in case of empty list

9fe6c93

comment

3ef5392

ArchibaldBienetre approved these changes Jan 23, 2022

View reviewed changes

ArchibaldBienetre marked this pull request as ready for review January 23, 2022 16:37

DonMischo and others added 2 commits January 24, 2022 21:30

replacements

6199185

fix: default grade row command

fix failing test

b4172d7

ArchibaldBienetre reviewed Jan 25, 2022

View reviewed changes

make test work on German-localized Ubuntu

481d95f

I don't know if this will work, @DonMischo must test

ArchibaldBienetre force-pushed the replacements branch from cf36060 to 481d95f Compare January 25, 2022 08:06

ArchibaldBienetre reviewed Jan 25, 2022

View reviewed changes

ArchibaldBienetre merged commit af489f9 into main Jan 30, 2022

ArchibaldBienetre deleted the replacements branch January 30, 2022 19:15

This was referenced Feb 12, 2022

Add absence days + hours handling to CSV format + replacements code #71

Merged

Add smoke test including real template #72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generation of Latex code and replacement in template #60

generation of Latex code and replacement in template #60

DonMischo commented Jan 7, 2022

ArchibaldBienetre commented Jan 7, 2022

ArchibaldBienetre commented Jan 7, 2022 •

edited

Loading

ArchibaldBienetre Jan 7, 2022 •

edited

Loading

ArchibaldBienetre commented Jan 9, 2022

ArchibaldBienetre Jan 21, 2022

ArchibaldBienetre Jan 21, 2022

ArchibaldBienetre Jan 21, 2022

ArchibaldBienetre Jan 21, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 21, 2022 •

edited

Loading

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022 •

edited

Loading

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre Jan 23, 2022

ArchibaldBienetre left a comment

ArchibaldBienetre Jan 25, 2022

ArchibaldBienetre Jan 25, 2022

ArchibaldBienetre commented Jan 26, 2022

ArchibaldBienetre commented Jan 30, 2022

generation of Latex code and replacement in template #60

generation of Latex code and replacement in template #60

Conversation

DonMischo commented Jan 7, 2022

ArchibaldBienetre commented Jan 7, 2022

ArchibaldBienetre commented Jan 7, 2022 • edited Loading

ArchibaldBienetre Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

ArchibaldBienetre commented Jan 9, 2022

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

ArchibaldBienetre Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArchibaldBienetre Jan 23, 2022 • 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

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

ArchibaldBienetre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArchibaldBienetre commented Jan 26, 2022

ArchibaldBienetre commented Jan 30, 2022

ArchibaldBienetre commented Jan 7, 2022 •

edited

Loading

ArchibaldBienetre Jan 7, 2022 •

edited

Loading

ArchibaldBienetre Jan 21, 2022 •

edited

Loading

ArchibaldBienetre Jan 23, 2022 •

edited

Loading