bpo-32143: add f_fsid to os.statvfs()#4571
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
giuseppe
force-pushed
the
statvfs_add_f_fsid
branch
from
31ed034 to
69c3227
Compare
There was a problem hiding this comment.
Is the cast to (unsigned long) necessary? Unnecessary casts can mask compiler warnings.
This could also be factored outside the #if block.
There was a problem hiding this comment.
no, I have only tried to keep the same pattern used for the other items. I will drop the cast and move the code outside of the #if block.
There was a problem hiding this comment.
I don’t understand this change at all. Does the behaviour of the original version change? It looks like this code won’t fail the test if the constructor succeeds anyway. Maybe better to use TestCase.assertRaises (if this needs testing at all).
There was a problem hiding this comment.
I've increased the size of the test by one since I've changed the statvfs return size by one, so to keep the same difference. Anyway I agree it makes no sense, and I will drop it.
giuseppe
force-pushed
the
statvfs_add_f_fsid
branch
4 times, most recently
from
468c74c to
457c952
Compare
|
@vadmium thanks for the review. I've addressed your comments and pushed a new version here ⬆️ |
giuseppe
force-pushed
the
statvfs_add_f_fsid
branch
from
457c952 to
7790048
Compare
|
@vadmium rebased and force pushed to retrigger the CLA check |
|
can this be reviewed? |
freddrake
left a comment
freddrake
left a comment
There was a problem hiding this comment.
Aside from these fairly straightforward changes, this looks good to me.
There was a problem hiding this comment.
Should probably verify that the value is an integer as well:
self.assertIsInstance(result.f_fsid, int)There was a problem hiding this comment.
Really minor nit: closing } should be aligned with those above for local consistency.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
giuseppe
force-pushed
the
statvfs_add_f_fsid
branch
from
7790048 to
0e2a95e
Compare
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @freddrake: please review the changes made to this pull request. |
|
I don't understand the problem with the Windows build; it doesn't appear related, but I never look at Windows build output. |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
statvfs_add_f_fsid
branch
from
0e2a95e to
666fcbb
Compare
|
@freddrake thanks for the quick review. I've rebased it, let's see if it makes any difference |
|
@vadmium: I'm planning to land this if AppVeyor is happy, unless you have objections. I don't see an approval from you for the changes you requested. |
|
@vadmium: Assuming you've no objections, since it's daytime in Australia. |
5 participants
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com
https://bugs.python.org/issue32143