-
Notifications
You must be signed in to change notification settings - Fork 2
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
1094 select proforma version for export direct download api #1550
1094 select proforma version for export direct download api #1550
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1550 +/- ##
=======================================
Coverage 94.13% 94.14%
=======================================
Files 123 123
Lines 3000 3004 +4
=======================================
+ Hits 2824 2828 +4
Misses 176 176 ☔ View full report in Codecov by Sentry. |
1b7cf97
to
fa49748
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work already! If you don't mind, here is some feedback already (besides the "draft" status of the PR). I tested the functionality with CodeOcean already and it worked flawlessly with either version 😁
4e8a25f
to
16f17e3
Compare
@MrSerth thanks for the feedback, I fixed your suggestions. I'm still waiting to merge the proforma PR, so we can also fix those edge-cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for integrating my feedback already. Here's some follow-up feedback I found, but otherwise I am fine with the changes.
For sure, I'll remind @Dome-GER about the missing review for openHPI/proformaxml#409 to unblock the current PR.
16f17e3
to
ece2929
Compare
Since openHPI/proformaxml#409 was merged, we can now draft a new ProFormA version, update this PR and undraft it. |
ece2929
to
9ca9877
Compare
9ca9877
to
1fbba56
Compare
@MrSerth Do you think we need to address the case of more ProformaXML version being added? Currently it is assumed that every saved task has the latest version. If a new version gets added that could lead to problems. A possible solution could be to add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrSerth Do you think we need to address the case of more ProformaXML version being added?
Not necessarily. The ProFormA working group is currently not working on a new version, so that I don't expect major changes in the near future (there could be a bugfix release, of course).
Currently it is assumed that every saved task has the latest version. If a new version gets added that could lead to problems.
I see. I would actually keep that assumption and would ensure that all existing tasks are migrated to a newer version when we add the respective version support in the future.
A possible solution could be to add a
proforma_version
column on task. What do you think?
Sure, could work. However, as mentioned above, I would not do so right now.
1fbba56
to
6b829d6
Compare
depends on openHPI/proformaxml#409