fix(torrent): `get_files()` should throw `KeyError` if `files` not fetched

[dechamps]

BREAKING CHANGE: torrent.get_files() will raise a KeyError if field files not fetched. file.priority and file.selected are not optional, based on fields fetched.

[trim21]

I'm considering this https://github.com/trim21/transmission-rpc/pull/422

This method should raise KeyError if files not fetched.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.64%. Comparing base (57c064f) to head (74d862b). Report is 94 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   77.67%   77.64%   -0.03%     
==========================================
  Files          14       14              
  Lines        1505     1503       -2     
==========================================
- Hits         1169     1167       -2     
  Misses        336      336              

Flag	Coverage Δ
3.10	77.64% <100.00%> (-0.03%)	:arrow_down:
3.11	77.64% <100.00%> (-0.03%)	:arrow_down:
3.12	77.64% <100.00%> (-0.03%)	:arrow_down:
3.8	77.44% <100.00%> (-0.04%)	:arrow_down:
3.9	77.44% <100.00%> (-0.04%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dechamps]

I'm considering this #422

I commented on that PR - I don't think it makes sense to force users to fetch fields they don't need.

This method should raise KeyError if files not fetched.

I agree that would make sense. Would you like me to extend this PR to also raise KeyError if files is not present?

[trim21]

I'm considering this #422

I commented on that PR - I don't think it makes sense to force users to fetch fields they don't need.

This method should raise KeyError if files not fetched.

I agree that would make sense. Would you like me to extend this PR to also raise KeyError if files is not present?

both sounds reasonable to me. But I plan to make breaking change when tr 4.1.0 is release.

Would mind update PR and wait until tr 4.1.0 is released? I'll merge it then.

[dechamps]

I plan to make breaking change when tr 4.1.0 is release

I'm not sure why you see this as a breaking change? With the previous code the only way to get get_files() to work and give the correct output was to fetch files, properties and wanted. This PR does not change the behavior in that case. It only changes the behavior for usage patterns where the previous code did not work at all in the first place. Can't break what's already broken.

Would mind update PR

I have updated the PR to make get_files() raise KeyError if files was not fetched.

[dechamps]

Adjusted the PR to account for CI failures, which I didn't notice until now because it didn't run on my development branch (see #448 about that).

[trim21]

I plan to make breaking change when tr 4.1.0 is release

I'm not sure why you see this as a breaking change? With the previous code the only way to get get_files() to work and give the correct output was to fetch files, properties and wanted. This PR does not change the behavior in that case. It only changes the behavior for usage patterns where the previous code did not work at all in the first place. Can't break what's already broken.

obviously it will throw KeyError now

[dechamps]

obviously it will throw KeyError now

It will not throw KeyError in code that is not already broken.

If a caller called this method without fetching files first, the previous code would return an incorrect result (i.e. an empty list). Meaning it's already broken.

The new code takes this already broken case and breaks it in a different, clearer way, by raising an error.

Caller code that is correct (i.e. callers who fetch files before calling get_files()) will not be broken by this change. The change only breaks callers that are already broken in the first place.

[trim21]

obviously it will throw KeyError now

It will not throw KeyError in code that is not already broken.

If a caller called this method without fetching files first, the previous code would return an incorrect result (i.e. an empty list). Meaning it's already broken.

The new code takes this already broken case and breaks it in a different, clearer way, by raising an error.

Caller code that is correct (i.e. callers who fetch files before calling get_files()) will not be broken by this change. The change only breaks callers that are already broken in the first place.

I get your point. To be honest, I had same conversation in some other repos with their maintainer.

And yes, you are right. calling .files() without files field fetched is actually not valid call, but changing it to raise a execption will have control flow problem.

Consider a long running script just call files and iter it, without try/catch.

It may get broken by this change, and I want to avoid this.

Also, there is already a in-plan breaking change so make a change like this in a non-major version change sounds like a realy bad idea to me.