Skip to content

Conversation

@rbevers
Copy link

@rbevers rbevers commented Feb 25, 2019

  • Adds a size attribute to FileObject to fulfill expectations of the Mezzanine 4.3.1 blog feed.
  • Adds a minimal set of unit tests to verify this addition is equivalent to filesize.

@rbevers rbevers self-assigned this Feb 25, 2019
@rbevers
Copy link
Author

rbevers commented Feb 25, 2019

Reproducing test case:

======================================================================
ERROR: test_size_is_filesize (filebrowser_safe.test_base.FileObjectTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rbevers/Work/bevy/filebrowser-safe/filebrowser_safe/test_base.py", line 13, in test_size_is_filesize
    self.assertEqual(fileobj.size, 17834)
AttributeError: FileObject instance has no attribute 'size'

@rbevers rbevers requested a review from abendig February 25, 2019 23:10
@rbevers
Copy link
Author

rbevers commented Feb 25, 2019

To test locally, I did the following:

$ ./test.sh
# Runs Mezzanine tests with this filebrowser version installed.
# ... lots of messy output, but no failures...
$ python setup.py tests
running test
running egg_info
writing filebrowser_safe.egg-info/PKG-INFO
writing top-level names to filebrowser_safe.egg-info/top_level.txt
writing dependency_links to filebrowser_safe.egg-info/dependency_links.txt
reading manifest file 'filebrowser_safe.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'filebrowser_safe.egg-info/SOURCES.txt'
running build_ext
test_size_is_filesize (filebrowser_safe.test_base.FileObjectTests) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.032s

OK

@rbevers rbevers assigned abendig and unassigned rbevers Feb 25, 2019
@rbevers rbevers marked this pull request as ready for review February 25, 2019 23:14
@rbevers
Copy link
Author

rbevers commented Feb 25, 2019

@abendig This has a CircleCI config, but won't let me configure it... "Contact repo admin"

screen shot 2019-02-25 at 3 09 39 pm

@rbevers
Copy link
Author

rbevers commented Feb 26, 2019

@abendig Ping.

@rbevers rbevers requested review from abendig and removed request for abendig February 26, 2019 19:35
@rbevers
Copy link
Author

rbevers commented Feb 26, 2019

Should we also have Chris in this repo? Assigned to you because no one else is here.

Copy link

@abendig abendig left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

I set up following on CI and added Chris to the team and this PR.

@rbevers
Copy link
Author

rbevers commented Feb 26, 2019

@ChrisAikman This repo may be unfamiliar. And it's not notifying Slack. Hope you see this.

@abendig abendig removed their assignment Feb 26, 2019
Copy link

@ChrisAikman ChrisAikman left a comment

Choose a reason for hiding this comment

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

Looks good! I have one question for my curiosity. 👍 :shipit:

setup(
name="filebrowser_safe",
version="0.5.0bevy2",
version="0.5.0.post3",

Choose a reason for hiding this comment

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

Curious: what is this version convention based on?

Copy link
Author

Choose a reason for hiding this comment

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

I learned, in trying to make the warnings about wrong dependency versions for Mezzanine go away, that Python packages have their own versioning scheme, specified in PEP 440.

Based on experimentation, a post release version was the only way to increment the version, but also satisfy the dependency on >=0.5.0 without a likely collision with a real-live release version. I wanted to get bevy in the version string but it gave warnings every time. Perhaps I didn't read the PEP carefully enough...

@ChrisAikman ChrisAikman assigned rbevers and unassigned ChrisAikman Feb 26, 2019
@ChrisAikman
Copy link

ChrisAikman commented Feb 26, 2019

Sorry, one more question. It seems the forked library is no longer maintained. Is the idea to keep patching the library as needed, or to eventually look elsewhere?

@rbevers
Copy link
Author

rbevers commented Feb 26, 2019

Not sure what led you to think it is no longer maintained. It doesn't get changes often. And we've forked the version that the Mezzanine guy forked from the original project. So it is a bit unclear to me how maintained it is.

That said, we are operating on an older version here because of the speed-up necessary for our Softlayer storage, so technically our 0.5.0.post3 is actually a flavor of 0.4.3 we've modified. The version fake is to make Mezzanine 4.3.1 happy.

@rbevers rbevers assigned ChrisAikman and unassigned rbevers Feb 26, 2019
Copy link

@ChrisAikman ChrisAikman left a comment

Choose a reason for hiding this comment

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

Sorry, somehow clicked Request changes instead of Approve. For real this time: 👍 :shipit:

@ChrisAikman ChrisAikman assigned rbevers and unassigned ChrisAikman Feb 26, 2019
@rbevers rbevers merged commit fca96c0 into remote_speed Feb 26, 2019
@rbevers rbevers deleted the SGP-6170/file-needs-size branch February 26, 2019 23:20
jacobhamblin pushed a commit that referenced this pull request May 20, 2020
Fix cs specification that is breaking compilemessages command
jacobhamblin pushed a commit that referenced this pull request May 21, 2020
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.

4 participants