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

upgrade to fsdb v0.3.2 #133

Merged
merged 1 commit into from
Apr 17, 2015
Merged

upgrade to fsdb v0.3.2 #133

merged 1 commit into from
Apr 17, 2015

Conversation

ael-code
Copy link
Member

No description provided.

@boyska
Copy link
Member

boyska commented Mar 24, 2015

well, I don't think this is a great approach. I think that this is the right moment to polish the code and make it unit-testable. Right now, it's not. I have really no hurry in upgrading to fsdb v0.3.2, and prefer to wait a bit and have better code quality in libreant than just "patching" the new method names.

The point is that a real, meaningful migration to fsdb v0.3 also means using buffers instead of paths. Doing so is a not-so-small change, so proper testing is required. Look at my branch (I left a comment on #132 about it), I hope you like it.

@ael-code
Copy link
Member Author

well, I don't think this is a great approach

No. It's necessary. Fsdb had a big bug in the previous version.

I think that this is the right moment to polish the code and make it unit-testable. Right now, it's not. I have really no hurry in upgrading to fsdb v0.3.2, and prefer to wait a bit and have better code quality in libreant than just "patching" the new method names.

polish the code and make it unit-testable have nothing to do with this commit, really nothing.

The point is that a real, meaningful migration to fsdb v0.3 also means using buffers instead of paths.

In some function that we use paths are better then streams: fsdb has some optimization for paths (buffer sizing) and flask does not provide some functionality with streams. In addition it's easy to mess up things with seekable streams.

This is a very small change that keeps things working without bugs. Please consider merging it.

@boyska
Copy link
Member

boyska commented Mar 24, 2015

On Tue, Mar 24, 2015 at 02:36:34PM -0700, ael wrote:

well, I don't think this is a great approach

No. It's necessary. Fsdb had a big bug in the previous version.

which one? so maybe we should add the "bug" label to #132

I think that this is the right moment to polish the code and make it unit-testable. Right now, it's not. I have really no hurry in upgrading to fsdb v0.3.2, and prefer to wait a bit and have better code quality in libreant than just "patching" the new method names.

polish the code and make it unit-testable have nothing to do with this commit, really nothing.

indeed :) the point is that I don't want to lose such an occasion. If
you promise ( :P ) to be willing to work on those goals anyway, this
pull request seems fine to me.

The point is that a real, meaningful migration to fsdb v0.3 also means using buffers instead of paths.

In some function that we use paths are better then streams: fsdb has some optimization for paths (buffer sizing) and flask does not provide some functionality with streams. In addition it's easy to mess up things with seekable streams.

You are talking about the X-Sendfile feature? is it important at all?
and what do you mean by "easy to mess up things" ? I don't think that
for example it is difficult to use buffers when offering book download.

@ael-code
Copy link
Member Author

On 03/24/2015 10:49 PM, BoySka wrote:

On Tue, Mar 24, 2015 at 02:36:34PM -0700, ael wrote:

well, I don't think this is a great approach

No. It's necessary. Fsdb had a big bug in the previous version.

which one? so maybe we should add the "bug" label to #132

ael-code/pyFsdb@4469a51

I think that this is the right moment to polish the code and make it unit-testable. Right now, it's not. I have really no hurry in upgrading to fsdb v0.3.2, and prefer to wait a bit and have better code quality in libreant than just "patching" the new method names.

polish the code and make it unit-testable have nothing to do with this commit, really nothing.

indeed :) the point is that I don't want to lose such an occasion. If
you promise ( :P ) to be willing to work on those goals anyway, this
pull request seems fine to me.

I promise.

The point is that a real, meaningful migration to fsdb v0.3 also means using buffers instead of paths.

In some function that we use paths are better then streams: fsdb has some optimization for paths (buffer sizing) and flask does not provide some functionality with streams. In addition it's easy to mess up things with seekable streams.

You are talking about the X-Sendfile feature? is it important at all?

I'm talking about X-Sendfile and mimetype guessing. They are not so
important, it was just an example.

@boyska
Copy link
Member

boyska commented Mar 25, 2015

No. It's necessary. Fsdb had a big bug in the previous version.

which one? so maybe we should add the "bug" label to #132

ael-code/pyFsdb@4469a51

ok, I add the 'bug' label. This changes everything!

By the way, I believe (but I am not sure) that a release that needs to
update the dependencies should not be a "patch" (as in
http://semver.org/ ), but at least a minor. So I propose that after your
commit get merged, we will bump to 0.2

The point is that a real, meaningful migration to fsdb v0.3 also means using buffers instead of paths.

In some function that we use paths are better then streams: fsdb has some optimization for paths (buffer sizing)
I'm talking about X-Sendfile and mimetype guessing. They are not so
important, it was just an example.

mimetype guessing is slightly more important, IMHO, so ok.

summing it up: I will soon test your commit and, if it behaves
correctly, I will merge. After that I will rebase my refactoring and
submit it to your attention.

@ael-code
Copy link
Member Author

summing it up: I will soon test your commit and, if it behaves correctly, I will merge. After that I will rebase my refactoring and submit it to your attention.

waiting :)

@boyska
Copy link
Member

boyska commented Apr 14, 2015

summing it up: I will soon test your commit and, if it behaves correctly, I will merge.

waiting :)

I'm sorry, I'm really busy in this period. If anyone else wants to step
ahead and test your commit, I'll appreciate this.
Otherwise, please continue to wait... and ping me from time to time!

upFile.save(tmpFilePath)
fileInfo = {}
fileInfo['name'] = secure_filename(upFile.filename)
fileInfo['size'] = os.path.getsize(tmpFilePath)
fileInfo['mime'] = upFile.mimetype
fileInfo['notes'] = request.form[upName+'_notes']
fileInfo['sha1'] = Fsdb.fileDigest(tmpFilePath, algorithm="sha1")
fileInfo['sha1'] = calc_file_digest(tmpFilePath, algorithm="sha1")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have called this simply file_digest and digest, but antani :P

@ael-code
Copy link
Member Author

both your comments are correct but related to fsdb and not to libreant.
Anyway did you test it? Can we merge it?

@boyska
Copy link
Member

boyska commented Apr 17, 2015

As discussed yesterday, assigned to @leophys

Un applauso per l'accollo!

@hellais
Copy link
Contributor

hellais commented Apr 17, 2015

👏 👏 🎉

@leophys
Copy link
Member

leophys commented Apr 17, 2015

Tested. It works.

@leophys leophys closed this Apr 17, 2015
@leophys leophys reopened this Apr 17, 2015
leophys added a commit that referenced this pull request Apr 17, 2015
@leophys leophys merged commit 7215ecc into insomnia-lab:master Apr 17, 2015
@ael-code ael-code deleted the to_fsdb_0.3 branch May 2, 2015 06:56
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.

None yet

4 participants