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

Big refactoring code and tests #18

Closed
wants to merge 20 commits into from
Closed

Conversation

klinkin
Copy link

@klinkin klinkin commented Jan 28, 2014

  • fix Optimization #16
  • prepare to be compatible with py3
  • use coverage and coveralls.io
  • using tqdm for print progress coping files (if log level = Info)
  • fix by autopep8

All existing tests are passed.

Trim EOL whitespace and other PEP8 issue by autopep8
Use tqdm for progress bar if log level=INFO
Converts statements of the form

    '%s' % foo

to statements of the form

    '{0}'.format(foo)

This completes removal of code necessary for Python 2.5 support; Flask no longer support Python 2.5.
Tox
Fix log-capture
@eriktaubeneck
Copy link
Collaborator

Thanks @klinkin. I've made a couple comments on the two issues, but other than that it mostly looks good.

The tqmd thing cool, but I'm not sure that the value outweighs issues it may cause when people use this for deployment on arbitrary systems that are using log feeds and what not. @e-dard any opinion on this?

@e-dard
Copy link
Owner

e-dard commented Jan 30, 2014

Will chip in tomorrow, if time permits 😃

@e-dard
Copy link
Owner

e-dard commented Feb 9, 2014

Can we revert the assert calls back to assertFoo where appropriate, please?

logger.debug("All valid files: %s" % all_files)
conn = S3Connection(user, password) # connect to s3
for (static_folder, static_url_loc), files in all_files.iteritems():
logger.debug('{0} valid files in folder "{1}" with local url "{2}"'.format(len(files), static_folder, static_url_loc))
Copy link
Owner

Choose a reason for hiding this comment

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

Can we get this down to 80 chars please?

Copy link
Author

Choose a reason for hiding this comment

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

Now problem!

@e-dard
Copy link
Owner

e-dard commented Feb 10, 2014

No idea what it is, so I'll defer to your judgement.

On 30 Jan 2014, at 22:15, Erik Taubeneck [email protected] wrote:

Thanks @klinkin. I've made a couple comments on the two issues, but other than that it mostly looks good.

The tqmd thing cool, but I'm not sure that the value outweighs issues it may cause when people use this for deployment on arbitrary systems that are using log feeds and what not. @e-dard any opinion on this?


Reply to this email directly or view it on GitHub.

@e-dard
Copy link
Owner

e-dard commented May 2, 2014

can this be merged @eriktaubeneck ?

@eriktaubeneck
Copy link
Collaborator

Sorry, let this one slip. I'm still not a fan of adding the tqmd requirement. Also, the refactor of the __version__ etc. into flask_s3.py with the regex look up in setup.py makes me feel funny. The author of webassets does this as well and it's only caused confusion and problems for people.

@e-dard
Copy link
Owner

e-dard commented May 2, 2014

@klinkin closing this, but happy to re-open if @eriktaubeneck's concerns are addressed.

@e-dard e-dard closed this May 2, 2014
@eriktaubeneck
Copy link
Collaborator

@klinkin if there is a good reason for the __version__ stuff, I'm all ears. Maybe I'm missing something, but generally I like to stick to "explicit is better than implicit".

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.

Optimization
3 participants