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

SubmissionCollection moves #231

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Conversation

Alberth289346
Copy link
Contributor

@Alberth289346 Alberth289346 commented Nov 21, 2021

Introducing a SubmissionCollection and moving common part of submission parsing to Submission.

This is stacked on top of #230 but much bigger, and it may need some tweaking or discussing before it's good. Don't merge before #230 I'd say.

As explained in #230, I am having a fight with Java versions and maven.

I didn't do testing beyond running maven that includes some tests.

@Alberth289346 Alberth289346 marked this pull request as draft November 21, 2021 16:07
@Alberth289346
Copy link
Contributor Author

Apparently it throws #230 and this patch onto one heap of changes, making the changes unreadable :(
Don't know how to fix other than wait until #230 is merged,

Marked it as draft to avoid accidental merge, however if you're happy with the changes, feel free to convert it back to ready.

@tsaglam tsaglam self-assigned this Nov 22, 2021
@tsaglam tsaglam added the enhancement Issue/PR that involves features, improvements and other changes label Nov 22, 2021
@tsaglam
Copy link
Member

tsaglam commented Nov 22, 2021

Maybe you can update the PR, now that #230 is merged.
Also, note that I turned the our discussion regarding submission filtering into issue #232, which might be also relevant for this PR.

@Alberth289346
Copy link
Contributor Author

New version, less involved than the previous try.

I contemplated whether to move the JPlag parsing code into SubmissionSet, but that likely isn't going to be very useful with all the links back to the Program interface of the JPlag class.

@Alberth289346 Alberth289346 marked this pull request as ready for review November 23, 2021 12:53
@tsaglam
Copy link
Member

tsaglam commented Nov 23, 2021

I contemplated whether to move the JPlag parsing code into SubmissionSet, but that likely isn't going to be very useful with all the links back to the Program interface of the JPlag class.

I like the changes in this PR. I checked out this branch and I think I found a way to reasonably move most of the parsing code into the submission set (basically separating the error reporting capabilities defined by Program into an own class). I will try to fully implement that on top of your changes and then we can maybe discuss them here.

@Alberth289346
Copy link
Contributor Author

Alberth289346 commented Nov 23, 2021

WIP: https://github.com/Alberth289346/jplag/compare/subm-collection-moves...Alberth289346:frutsel-program?expand=1

I had a go too, created a new ProgramProxy, acting as Program for the language parsers, then started hacking the implements Program out of JPlag. Also pulled Language language out of options as I didn't see much need for it there, but report now dies :p

@tsaglam
Copy link
Member

tsaglam commented Nov 24, 2021

I had a go too, created a new ProgramProxy, acting as Program for the language parsers, then started hacking the implements Program out of JPlag. Also pulled Language language out of options as I didn't see much need for it there, but report now dies :p

I also removed the error collecting from the program and changed the language frontend dependencies to no longer reference the main class. However, I think it is better to leave the language in the options, due to other features that are currently worked at. These changes do not affect the report generation. Maybe you can take a look at #234, if it is okay with you I would go ahead with that approach (so first merge #231 and then #234).

@tsaglam tsaglam merged commit ceea4fd into jplag:master Nov 24, 2021
@Alberth289346 Alberth289346 deleted the subm-collection-moves branch November 24, 2021 08:40
@tsaglam tsaglam added this to the v3.0.0 milestone Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants