Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Text Blocks #2122

Merged
merged 10 commits into from
Oct 13, 2020
Merged

Use Text Blocks #2122

merged 10 commits into from
Oct 13, 2020

Conversation

MaisiKoleni
Copy link
Contributor

@MaisiKoleni MaisiKoleni commented Sep 24, 2020

Checklist

  • This PR only changes test code
  • I checked that all Java tests still work on GitHub, Bamboo and locally.

Motivation and Context

We switched to Java 15 which offers text blocks, see https://openjdk.java.net/jeps/378.

Replace formatted, multiline strings that are hard to read by formatted text blocks.

Note: this excludes SQL/HQL strings which will be addressed in smaller follow-up PRs.

Description

Replaced the strings by text blocks. Currently, only strings that span multiple lines are changed.

Most of the changes affect only test code, with a large proportion of that being mock requests and alike.

This means that

"ABC\n" + "    xyz\n" +
"something long\n"

is replaced by

"""
ABC
    xyz
something long
"""

Some things you should know about text blocks

  • The position of the first """ introducing the text block has no effect, but it must be directly followed by a line break.
  • Line breaks in the text block are preserved by default and always of the LF (\n) type. The one after the """ does not count.
  • A line break can be prevented by ending the line with \.
  • Windows line endings can be archived by ending a line with \r (which is then followed by the implicit \n).
  • Whitespace at the end of the line is removed (unless an escaped space \s is added at the very end).
  • Single " and double "" do not need to be escaped in a text block.
  • A text block ends with """. Examples:
    "abc" == """
             abc"""
    
    "abc\n" == """
               abc
               """
    
    "  abc\n" == """
                 abc
               """
  • Whitespace at the start of each line is removed as much as it is possible to do consistently. This means that the minimal number of whitespace characters that any line starts with is then removed from all lines. (See e.g. the last example of the point above)

Steps for Testing

  1. Run the server tests on your system
  2. Check the code
  3. Check that most of the system still works on a test server / locally.

Note: "\" at the end of a line inside a text block prevents a new line \n from being inserted. This keeps the String on a single line while being formatted and well readable at the same time.
Note: \" is now replaced with " because escaping " is not required in a text block. JSON looks much more like JSON now.
Note: by ending each line with "\r", we can keep the Windows CRLF endings in the text block. Newlines in text blocks themselves are always just "\n".
@MaisiKoleni MaisiKoleni added work in progress server Pull requests that update Java code. (Added Automatically!) chore labels Sep 24, 2020
@MaisiKoleni MaisiKoleni self-assigned this Sep 24, 2020
@MaisiKoleni
Copy link
Contributor Author

MaisiKoleni commented Sep 25, 2020

I would also recommend using it for the HQL Queries:

    @Query("""
            SELECT c FROM Complaint c
               LEFT JOIN FETCH c.result r
               LEFT JOIN FETCH r.assessor
               LEFT JOIN FETCH r.participation p
               LEFT JOIN FETCH p.exercise e
               LEFT JOIN FETCH r.submission
            WHERE e.id = :#{#exerciseId} AND c.complaintType = :#{#complaintType}
            """)

or

    @Query("""
            SELECT count(c) FROM Complaint c
            WHERE c.complaintType = 'COMPLAINT'
              AND c.student.id = :#{#studentId}
              AND c.result.participation.exercise.course.id = :#{#courseId}
              AND (c.accepted = false OR c.accepted is null)""")

I started to do that with queries above 200 characters, in my eyes this is much more readable.

But before I continue, we could decide on a format here (how much indentation, where to indent).

More complex example and suggestion:

    @Query("""
            SELECT COUNT (DISTINCT p) FROM StudentParticipation p
            WHERE p.exercise.id = :#{#exerciseId}
              AND EXISTS (Select s FROM p.submissions s
                          where s.result Is not null
                            and exists (select c from Complaint c
                                        where c.result.id = s.result.id
                                          and c.complaintType = :#{#complaintType}))
              AND NOT EXISTS (select prs from p.results prs
                              where prs.assessor.id = p.student.id)
            """)

We could e.g. also place from on an extra line, as it is commonly done.

At least everything is better than just

@Query("select te from TextExercise te where (te.id in (select courseTe.id from TextExercise courseTe where courseTe.course.instructorGroupName in :groups and (courseTe.title like %:partialTitle% or courseTe.course.title like %:partialCourseTitle%)) or te.id in (select examTe.id from TextExercise examTe where examTe.exerciseGroup.exam.course.instructorGroupName in :groups and (examTe.title like %:partialTitle% or examTe.exerciseGroup.exam.course.title like %:partialCourseTitle%)))")

in my opinion.

@MaisiKoleni
Copy link
Contributor Author

MaisiKoleni commented Sep 25, 2020

Maybe we should also finally decide whether we want to use uppercase or lowercase.

At the moment we have SELECT, select and Select. Sometimes even all in one single query, as shown above (from ComplaintRepository).

@krusche
Copy link
Member

krusche commented Sep 25, 2020

Spotless could be a limitation here. I suggest you add an issue in their Github Repo and ask for support.
Please make sure that we do not need to add enable-preview here, this was a real pain in the end.

@krusche
Copy link
Member

krusche commented Sep 25, 2020

I found comments about the missing support of switch expressions e.g. here: google/google-java-format#477 You could add another issue here.

Another option would be to add it into https://github.com/diffplug/spotless/issues

I just updated spotless and check style to their latest versions on develop. You could give it a try. Maybe it works now.

@MaisiKoleni MaisiKoleni force-pushed the chore/use-text-blocks branch from 095b447 to 3957867 Compare October 9, 2020 16:39
@MaisiKoleni
Copy link
Contributor Author

We decided in the developer meeting to split the transformation of the SQL/HQL queries of the *Repository classes into smaller PRs. Therefore, such changes are out of scope for this PR and will be addressed in smaller follow-up PRs.

It so happens now that this PR only changes Java test code.

@MaisiKoleni MaisiKoleni marked this pull request as ready for review October 9, 2020 16:59
@MaisiKoleni MaisiKoleni requested review from jpbernius, krusche and a team as code owners October 9, 2020 16:59
@MaisiKoleni MaisiKoleni requested a review from sleiss October 9, 2020 16:59
@MaisiKoleni MaisiKoleni requested a review from a team October 9, 2020 16:59
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not make more sense to put the json strings of some of the test cases into actual json files in the folder src/test/resources/test-data
I don't think having those strings in Java is beneficial.
We already have a method loadFileFromResources in the tests that handles this.
(I refactored this method in another branch into a new Java class FileUtils)

Copy link

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 16
- Added 7
           

Coverage increased per file
===========================
+ src/main/webapp/app/entities/text-block-ref.model.ts  8
+ src/main/java/de/tum/in/www1/artemis/service/connectors/bamboo/BambooService.java  18
+ src/main/java/de/tum/in/www1/artemis/service/connectors/bamboo/dto/QueriedBambooBuildResultDTO.java  100
+ src/main/java/de/tum/in/www1/artemis/service/connectors/bamboo/dto/BambooTestResultDTO.java  100
+ src/main/webapp/app/exercises/quiz/shared/short-answer-question-util.service.ts  34
+ src/main/webapp/app/exercises/shared/submission/submission.service.ts  25
+ src/main/java/de/tum/in/www1/artemis/domain/participation/ProgrammingExerciseStudentParticipation.java  4
+ src/main/webapp/app/exercises/shared/exercise/exercise.service.ts  13
+ src/main/java/de/tum/in/www1/artemis/service/connectors/bamboo/dto/BambooChangesDTO.java  43
+ src/main/webapp/app/exercises/text/assess-new/manual-textblock-selection/manual-textblock-selection.component.ts  67
+ src/main/webapp/app/exercises/shared/structured-grading-criterion/grading-instructions-details/grading-instructions-details.component.ts  42
+ src/main/webapp/app/exercises/shared/team/team-exercise-search/team-exercise-search.component.ts  38
+ src/main/webapp/app/exercises/file-upload/participate/file-upload-submission.component.ts  1
+ src/main/java/de/tum/in/www1/artemis/service/connectors/bamboo/dto/BambooArtifactsDTO.java  100
+ src/main/webapp/app/exam/participate/exam-participation.service.ts  73
+ src/main/java/de/tum/in/www1/artemis/domain/BuildLogEntry.java  10
+ src/main/webapp/app/exercises/text/shared/text-select.directive.ts  21
+ src/main/java/de/tum/in/www1/artemis/domain/enumeration/StaticCodeAnalysisTool.java  16
+ src/main/webapp/app/entities/student-exam.model.ts  50
+ src/main/webapp/app/exercises/shared/participation/participation.service.ts  11
+ src/main/webapp/app/shared/service/sort.service.ts  81
         

Complexity increasing per file
==============================
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/classdiagram/UMLClassDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/deployment/UMLDeploymentDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/programmingexercise/ProgrammingSubmissionConstants.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/component/UMLComponentDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/object/UMLObjectDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/communication/UMLCommunicationDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/usecase/UMLUseCaseDiagrams.java  1
- src/test/java/de/tum/in/www1/artemis/service/compass/umlmodel/activity/UMLActivityDiagrams.java  1
         

See the complete overview on Codacy

@krusche krusche merged commit 9dc0d4f into develop Oct 13, 2020
@krusche krusche deleted the chore/use-text-blocks branch October 13, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants