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.

Sign up for GitHub

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

Jump to bottom

Add Golang frontend #500

Merged
merged 26 commits into from
Aug 3, 2022
Merged

Add Golang frontend #500

merged 26 commits into from
Aug 3, 2022

Conversation

robinmaisch
Copy link
Contributor

@robinmaisch robinmaisch commented Jul 10, 2022 •
edited
Loading

This PR introduces a Golang frontend to JPlag. Its structure is very similar to the C# and R frontends.
It comes with a README, some basic tests, and some test code in written in Go.

Also, the README of the top module is updated to reflect the new CLI language options.
Its grammar definition should comply to go1.17, the current version being go1.18.

Addressing #345.

@robinmaisch
Copy link
Contributor Author

The remaining indicated code smells have a point, but they are kind of by design/convention based on the rest of the project. I would argue they are acceptable for now.

JanWittler
JanWittler reviewed Jul 18, 2022
View reviewed changes
Copy link
Contributor

@JanWittler JanWittler left a comment

Choose a reason for hiding this comment

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

I added some comments regarding the token selection which seemed worth a discussion for me.
I am not an expert with Go, so there might be totally valid reasons to select the tokens as you did.

jplag.frontend.golang/src/main/java/de/jplag/golang/GoTokenConstants.java Outdated Show resolved Hide resolved
jplag.frontend.golang/src/main/java/de/jplag/golang/GoTokenConstants.java Outdated Show resolved Hide resolved
@JanWittler JanWittler requested a review from a team July 19, 2022 06:49
@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change language PR / Issue deals (partly) with new and/or existing languages for JPlag labels Jul 25, 2022
@tsaglam tsaglam added this to the v4.0.0 milestone Jul 25, 2022
@tsaglam tsaglam requested a review from a team July 28, 2022 13:10
@robinmaisch
Copy link
Contributor Author

@jplag/studdev @jplag/maintainer
The conflicting files are merged; the current commit is now ready for review.
Maybe put a thumbs up so that I know that you got the message, otherwise I'll write an e-mail.

@dfuchss
Copy link
Member

dfuchss commented Jul 29, 2022

The sonar cloud issue is known. I'm working on it in #538

@dfuchss
Copy link
Member

dfuchss commented Jul 29, 2022 •
edited
Loading

@robinmaisch can you merge the current master branch to your branch?

@robinmaisch
Copy link
Contributor Author

@robinmaisch can you merge the current master branch to your branch?

Done. It works, thanks!

SuyDesignz
SuyDesignz suggested changes Jul 31, 2022
View reviewed changes
jplag.frontend.golang/src/main/java/de/jplag/golang/GoParserAdapter.java Outdated Show resolved Hide resolved
jplag.frontend.golang/src/main/java/de/jplag/golang/Language.java Outdated Show resolved Hide resolved
jplag.frontend.golang/src/main/java/de/jplag/golang/grammar/GoParserBase.java Show resolved Hide resolved
jplag.frontend.golang/src/main/java/de/jplag/golang/grammar/GoTokenUtils.java Outdated Show resolved Hide resolved
jplag.frontend.golang/src/main/java/de/jplag/golang/grammar/GoTokenUtils.java Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
jplag.frontend.golang/README.md Show resolved Hide resolved
jplag.frontend.golang/pom.xml Show resolved Hide resolved
jplag.frontend.golang/src/main/antlr4/de/jplag/golang/grammar/GoLexer.g4 Show resolved Hide resolved
jplag.frontend.golang/src/main/antlr4/de/jplag/golang/grammar/GoParser.g4 Show resolved Hide resolved
tsaglam
tsaglam approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Looks good to me. Resolve the issues mentioned in the other reviews, and this is ready to merge!

dfuchss
dfuchss requested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

Please merge the current main branch to your branch :) error consumer has been removed. A logger is available now :)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

47.6% 47.6% Coverage
2.1% 2.1% Duplication

dfuchss
dfuchss approved these changes Aug 3, 2022
View reviewed changes
jplag.cli/pom.xml
Comment on lines 15 to 17
<dependency>
<groupId>de.jplag</groupId>
<artifactId>frontend-utils</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Add dependency to golang here

Copy link
Member

Choose a reason for hiding this comment

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

But that's something we can also do later

@tsaglam tsaglam merged commit 1ae04f2 into jplag:master Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsaglam tsaglam tsaglam approved these changes

@JanWittler JanWittler JanWittler approved these changes

@dfuchss dfuchss dfuchss approved these changes

@SuyDesignz SuyDesignz Awaiting requested review from SuyDesignz

Assignees
No one assigned
Labels
enhancement Issue/PR that involves features, improvements and other changes language PR / Issue deals (partly) with new and/or existing languages for JPlag minor Minor issue/feature/contribution/change
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@robinmaisch @dfuchss @tsaglam @JanWittler @SuyDesignz