-
Notifications
You must be signed in to change notification settings - Fork 398
Remove PrintingService #1583
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
Remove PrintingService #1583
Conversation
veluca93
left a comment
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.
Seems reasonable to me but if @prandla can give it a second look I would not mind :-)
|
On translations, I think we can get rid of them. |
dba424a to
30541a1
Compare
|
I thought we wanted to do a poll of existing users to see if they still use some of these "deprecated" features? printing was only broken in v1.5.0, it seems likely that some people might not have upgraded to it yet. and it's not hard to keep it working if someone does use it. (this feature is much less complicated than tokens for example...) regarding the pr: i'll need to pull translation updates from weblate first to avoid merge conflicts. i think cmscommon/tex.py is also useless now. |
|
also, i think your updater is buggy: you're mutating self.objs while iterating over it. |
I don't think we need a poll for PrintingService. |
|
I could see how some contest admins might want CMS to offer this functionality, instead of having to configure it correctly. Especially if the contestant machines aren't super uniform (e.g. multiple operating systems). DOMJudge offers this functionality as well, we could ask the developers if they have an opinion on this feature and if they have plans to deprecate it. |
|
We have had the issue about printing being broken for two months. On that issue, the only messages present have been from people saying that we should remove it. I think that was long enough time for someone to come forward if need be (in fact, it was probably more, since the feature was broken before). Given the cirumstances, I will merge this PR once it is technically ready. If someone complains once the feature is removed, we can restore it easily since we have access to git history. |
|
Fair, I guess it could even be re-implemented in an simpler way, if we'll want this in the future. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
+ Coverage 54.56% 54.66% +0.10%
==========================================
Files 339 335 -4
Lines 27653 27358 -295
==========================================
- Hits 15088 14956 -132
+ Misses 12565 12402 -163
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
You are right, I thought python was smarter, now fixed.
It seems you are right, removed. |
prandla
left a comment
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.
libcups should also be removed from Dockerfile.
I updated the translations; you should be able to rerun the babel extract to resolve the merge conflicts.
Thanks, conflicts resolved. I also updated the Dockerfile and the installation instruction removing the pointed out packages. |
Closes #1544.
As already discussed the PrintingService is unused and even broken.
This PR removes all (I hope) the relative code, this makes installation easier since it requires fewer dependencies.
I just left the translations marking them as deprecated (should we completely remove them?).