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

deprecates integration with PHP Copy/Paste Detector (PHPCPD) library. #264

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

stopfstedt
Copy link
Contributor

@stopfstedt stopfstedt commented Dec 14, 2023

deprecation as a stepping stone towards full removal.

i tried to be cheeky by triggering a deprecation error when the command is executed, but that didn't pan out.
so it's all documentation, and one @deprecated annotation on the command itself.

refs #262
refs #263

@stopfstedt stopfstedt force-pushed the deprecate_copy_paste_detector branch 2 times, most recently from e60ba80 to 5522142 Compare December 14, 2023 18:03
@stopfstedt stopfstedt changed the title deprecates integration PHP Copy/Paste Detector (PHPCPD) library. deprecates integration with PHP Copy/Paste Detector (PHPCPD) library. Dec 14, 2023
@stopfstedt stopfstedt force-pushed the deprecate_copy_paste_detector branch 2 times, most recently from c946066 to 2d64817 Compare January 9, 2024 17:25
@stopfstedt stopfstedt marked this pull request as ready for review January 9, 2024 18:02
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (0aef673) 85.10% compared to head (02cb44e) 84.77%.

Files Patch % Lines
src/Command/CopyPasteDetectorCommand.php 18.18% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #264      +/-   ##
============================================
- Coverage     85.10%   84.77%   -0.34%     
- Complexity      718      720       +2     
============================================
  Files            75       75              
  Lines          2216     2226      +10     
============================================
+ Hits           1886     1887       +1     
- Misses          330      339       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stronk7
Copy link
Member

stronk7 commented Feb 4, 2024

Thanks @stopfstedt (and sorry for the late reply, I've been distracted with other stuff).

Your patch looks ok, only detail that, maybe, we could try is to print some warning as an annotation in the workflow. I've not tried that myself, but think that it can be an easy and effective way to inform to everybody about the deprecation (if it works).

Also, I've seen that the CLI.md (that is auto-generated), does include the (DEPRECATED) information. Has that been manually added by you or it's automatic Symfony stuff that adds it for deprecated commands? I can check, heh, but asking is cheaper, TIA!

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Feb 4, 2024

Hi again, @stopfstedt.

I’ve taken the liberty of adding one extra commit on top of your changes (squashed to just 1 commit, for clarity).

Here you can see the proposed changes: main...stronk7:moodle-plugin-ci:deprecate_copy_paste_detector

And here you can see how it will look for anybody running the command via GitHub workflows, with annotations reminding about the deprecation all the time:

https://github.com/stronk7/moodle-plugin-ci/actions/runs/7775806197

Tell me what do you think and, if you like it, I will add the reworked commits to your branch, so anybody else in the team, can take a look to it.

Ciao 😊

@stronk7 stronk7 added the question Further information is requested label Feb 4, 2024
@stopfstedt
Copy link
Contributor Author

Tell me what do you think and, if you like it, I will add the reworked commits to your branch, so anybody else in the team, can take a look to it.

This is great, please go ahead. Thanks!

@stronk7 stronk7 force-pushed the deprecate_copy_paste_detector branch from 2d64817 to 292b52c Compare February 4, 2024 22:43
@stronk7
Copy link
Member

stronk7 commented Feb 4, 2024

This is great, please go ahead. Thanks!

To you! I've forced-updated your branch and now it contains the 2 commits described above.

Note (for the reviewer) that it's expected for this change to cause a slight decrease in PHPUnit code coverage, but that is expected and on-purpose, because we don't want the 2 warnings (Symfony and GHA) to be emitted when running tests.

Ciao :-)

@stronk7 stronk7 removed the question Further information is requested label Feb 4, 2024
stopfstedt and others added 2 commits February 4, 2024 23:58
- Add details to changelog.
- Emit a Symfony deprecation (but if running unit tests).
- Add a GH Workflow annotation to warn devs.

Note that, as a result of the changes in this commit,
code coverage will slightly decrease, because some
of the changes are unreachable when running unit tests,
but that's by design and on purpose, should be acceptable.
@stronk7 stronk7 force-pushed the deprecate_copy_paste_detector branch from 292b52c to 02cb44e Compare February 4, 2024 22:58
Copy link
Member

@paulholden paulholden left a comment

Choose a reason for hiding this comment

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

LGTM

@stronk7
Copy link
Member

stronk7 commented Feb 5, 2024

Thanks @paulholden , merging this now. And thanks too @stopfstedt !

@stronk7 stronk7 merged commit 8124fe7 into moodlehq:main Feb 5, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants