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.

Sign up for GitHub

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

Jump to bottom

bpo-32143: add f_fsid to os.statvfs() #4571

Merged
merged 1 commit into from
Dec 14, 2017
Merged

bpo-32143: add f_fsid to os.statvfs() #4571

merged 1 commit into from
Dec 14, 2017

Conversation

giuseppe
Copy link
Contributor

@giuseppe giuseppe commented Nov 26, 2017 •
edited by bedevere-bot
Loading

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

https://bugs.python.org/issue32143

@the-knights-who-say-ni
Copy link

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!

vadmium
vadmium reviewed Nov 26, 2017
View reviewed changes
Modules/posixmodule.c Outdated
@@ -9305,6 +9306,7 @@ _pystatvfs_fromstructstatvfs(struct statvfs st) {
PyStructSequence_SET_ITEM(v, 7, PyLong_FromLong((long) st.f_favail));
PyStructSequence_SET_ITEM(v, 8, PyLong_FromLong((long) st.f_flag));
PyStructSequence_SET_ITEM(v, 9, PyLong_FromLong((long) st.f_namemax));
PyStructSequence_SET_ITEM(v, 10, PyLong_FromUnsignedLong((unsigned long) st.f_fsid));
Copy link
Member

Choose a reason for hiding this comment

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

Is the cast to (unsigned long) necessary? Unnecessary casts can mask compiler warnings.

This could also be factored outside the #if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Lib/test/test_os.py Outdated
@@ -374,7 +374,7 @@ def test_statvfs_attributes(self):

# Use the constructor with a too-long tuple.
try:
result2 = os.statvfs_result((0,1,2,3,4,5,6,7,8,9,10,11,12,13,14))
result2 = os.statvfs_result((0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15))
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 giuseppe force-pushed the statvfs_add_f_fsid branch 4 times, most recently from 468c74c to 457c952 Compare November 27, 2017 06:55
@giuseppe
Copy link
Contributor Author

@vadmium thanks for the review. I've addressed your comments and pushed a new version here ⬆️

@giuseppe
Copy link
Contributor Author

@vadmium rebased and force pushed to retrigger the CLA check

@giuseppe
Copy link
Contributor Author

can this be reviewed?

freddrake
freddrake requested changes Dec 13, 2017
View reviewed changes
Copy link
Member

@freddrake freddrake left a comment

Choose a reason for hiding this comment

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

Aside from these fairly straightforward changes, this looks good to me.

Lib/test/test_os.py Outdated
@@ -352,6 +352,11 @@ def test_statvfs_attributes(self):
for value, member in enumerate(members):
self.assertEqual(getattr(result, 'f_' + member), result[value])

self.assertTrue(hasattr(result, 'f_fsid'))
Copy link
Member

Choose a reason for hiding this comment

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

Should probably verify that the value is an integer as well:

self.assertIsInstance(result.f_fsid, int)

Modules/posixmodule.c Outdated
@@ -1860,6 +1860,7 @@ static PyStructSequence_Field statvfs_result_fields[] = {
{"f_favail", },
{"f_flag", },
{"f_namemax",},
{"f_fsid", },
Copy link
Member

Choose a reason for hiding this comment

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

Really minor nit: closing } should be aligned with those above for local consistency.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@giuseppe
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@freddrake: please review the changes made to this pull request.

@freddrake
Copy link
Member

I don't understand the problem with the Windows build; it doesn't appear related, but I never look at Windows build output.

@giuseppe
bpo-32143: add f_fsid to os.statvfs()
666fcbb 
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Contributor Author

@freddrake thanks for the quick review. I've rebased it, let's see if it makes any difference

@freddrake
Copy link
Member

@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.

@freddrake
Copy link
Member

@vadmium: Assuming you've no objections, since it's daytime in Australia.

@freddrake freddrake merged commit 96a5e50 into python:master Dec 14, 2017
@erickpeirson erickpeirson mentioned this pull request Jan 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadmium vadmium vadmium left review comments

@freddrake freddrake freddrake approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@giuseppe @the-knights-who-say-ni @bedevere-bot @freddrake @vadmium