-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add PairwiseDocNewUIESA Page #161
base: develop
Are you sure you want to change the base?
Conversation
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 this PR! It looks good, but I have a few small suggestions before we merge:
- We can drop "-newui" or "NewUI" from the class/method/URL names for simplicity.
- I would like you to try introducing the new model class through inheritance instead of copy-pasting the entire class. I believe it's doable, but please feel free to object.
- We should add a new regression test in
RegressionTests/tests/examples
to make sure we don't break it in future. It may be helpful to do it before addressing the points above. - Can you also briefly check if none of the PRs pulled today to the develop branch didn't change any logic in ESA tasks, which you use here?
I will then take another look. Thank you :)
EvalView/templates/EvalView/pairwise-assessment-document-newui-esa.html
Outdated
Show resolved
Hide resolved
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.
Thank you very much for this PR! Everything seems to look good. Can you only summarize changes in the model file here (see one of the comments)? I think it may be useful for future reference.
I'd like to merge it after we finish WMT campaigns, which should be very soon.
Add PairwiseDocNewUIESA Page