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

Add an option to bin/fetch to exclude bodies. #3804

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Conversation

dracos
Copy link
Member

@dracos dracos commented Feb 16, 2022

No description provided.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #3804 (c78168f) into master (c626fa7) will decrease coverage by 22.65%.
The diff coverage is 50.00%.

❗ Current head c78168f differs from pull request most recent head 64a1a8c. Consider uploading reports for the commit 64a1a8c to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3804       +/-   ##
===========================================
- Coverage   82.33%   59.68%   -22.66%     
===========================================
  Files         352      284       -68     
  Lines       23969    19381     -4588     
  Branches     3650     3643        -7     
===========================================
- Hits        19736    11568     -8168     
- Misses       3086     6593     +3507     
- Partials     1147     1220       +73     
Impacted Files Coverage Δ
perllib/Open311/GetServiceRequests.pm 78.94% <0.00%> (-2.14%) ⬇️
perllib/Open311/UpdatesBase.pm 86.44% <100.00%> (+0.23%) ⬆️
perllib/FixMyStreet/App/Form/Waste/Garden.pm 11.11% <0.00%> (-88.89%) ⬇️
perllib/FixMyStreet/App/Form/Wizard.pm 5.88% <0.00%> (-88.24%) ⬇️
perllib/FixMyStreet/DB/Result/AdminLog.pm 8.16% <0.00%> (-85.72%) ⬇️
perllib/FixMyStreet/App/Controller/Auth/Profile.pm 15.96% <0.00%> (-82.36%) ⬇️
perllib/FixMyStreet/App/Controller/Admin/Roles.pm 16.36% <0.00%> (-81.82%) ⬇️
.../FixMyStreet/App/Controller/Admin/ManifestTheme.pm 18.18% <0.00%> (-81.82%) ⬇️
...erllib/FixMyStreet/App/Controller/Questionnaire.pm 11.45% <0.00%> (-80.16%) ⬇️
...ib/FixMyStreet/DB/Result/ModerationOriginalData.pm 10.00% <0.00%> (-78.75%) ⬇️
... and 188 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c626fa7...64a1a8c. Read the comment docs.

Copy link
Member

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

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

I'm afraid this isn't working as expected.

The option in bin/fetch is still called body, but the two .pm scripts are now looking for a bodies option. Similarly the exclude option isn't being passed through to the scripts at all, and would need to be called bodies_exclude at the point it was passed through.

Hopefully easy to fix both of those issues in the my %params declaration.

Would also be good to have a test for the bodies_exclude parameter, if that's not too tricky.

@dracos
Copy link
Member Author

dracos commented Feb 16, 2022

Not sure what I was thinking there! Thanks, fixup pushed with test.

Copy link
Member

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

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

Test looks good!

However I'm seeing errors when I run bin/fetch. I think the options passed from bin/fetch need some default values and/or the scripts need to check for undef and handle that correctly.

$ bin/fetch --body 'Rutland County Council' -v --reports
Can't use an undefined value as an ARRAY reference at perllib/Open311/GetServiceRequests.pm line 34.
$ bin/fetch --exclude 'Rutland County Council' -v --reports
Can't use an undefined value as an ARRAY reference at /Users/chrismytton/Code/mysociety/fixmystreet/perllib/Open311/GetServiceRequests.pm line 31.

Copy link
Member

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

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

As discussed in Slack it would ultimately be good to push more of the logic in bin/fetch into files in perllib. But that can be done as a separate change. This looks good to me! 🚀

@dracos dracos merged commit 64a1a8c into master Feb 16, 2022
@github-pages github-pages bot temporarily deployed to github-pages February 16, 2022 16:27 Inactive
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.

2 participants