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

Remove storify #744

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Remove storify #744

wants to merge 32 commits into from

Conversation

sl1-1
Copy link
Contributor

@sl1-1 sl1-1 commented Feb 17, 2020

Removes the need for web_input_request_method

@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2020

This pull request introduces 3 alerts when merging e8c49f0 into 620f84c - view on LGTM.com

new alerts:

  • 2 for Redundant assignment
  • 1 for Unused import

@codecov-io
Copy link

codecov-io commented Feb 17, 2020

Codecov Report

Merging #744 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #744   +/-   ##
=======================================
  Coverage   67.49%   67.49%           
=======================================
  Files         156      156           
  Lines       12450    12450           
=======================================
  Hits         8403     8403           
  Misses       4047     4047           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 222a53e...222a53e. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Feb 28, 2020

This pull request introduces 1 alert when merging 4a949b9 into 3588638 - view on LGTM.com

new alerts:

  • 1 for Syntax error

weasyl/controllers/general.py Outdated Show resolved Hide resolved
weasyl/controllers/interaction.py Outdated Show resolved Hide resolved
weasyl/controllers/moderation.py Outdated Show resolved Hide resolved
weasyl/controllers/general.py Outdated Show resolved Hide resolved
weasyl/controllers/api.py Outdated Show resolved Hide resolved
weasyl/controllers/api.py Outdated Show resolved Hide resolved
weasyl/controllers/content.py Outdated Show resolved Hide resolved
weasyl/controllers/content.py Outdated Show resolved Hide resolved
weasyl/controllers/content.py Outdated Show resolved Hide resolved
weasyl/controllers/moderation.py Outdated Show resolved Hide resolved
weasyl/controllers/moderation.py Outdated Show resolved Hide resolved
weasyl/moderation.py Outdated Show resolved Hide resolved
weasyl/report.py Outdated Show resolved Hide resolved
weasyl/test/login/test_create.py Outdated Show resolved Hide resolved
sl1-1 and others added 2 commits March 7, 2020 13:53
Copy link
Contributor

@charmander charmander left a comment

Choose a reason for hiding this comment

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

There are some .get('name', False)s that should probably just be .get('name') (since they were originally the equivalent of .get('name', '')), but we can fix that later.

It might be safer to review, merge, and deploy this one piece at a time…

if form.since:
since = d.parse_iso8601(form.since)
count = int(form.count)
if 'since' in request.params:
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make an empty ?since= will stop working. I think we may as well keep full backwards compatibility here.

return submission.select_view_api(
request.userid, int(request.matchdict['submitid']),
anyway=bool(form.anyway), increment_views=bool(form.increment_views))
anyway=('anyway' in request.params), increment_views=('increment_views' in request.params))
Copy link
Contributor

Choose a reason for hiding this comment

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

Behaviour difference for empty values ?anyway=, ?increment_views=.

Suggested change
anyway=('anyway' in request.params), increment_views=('increment_views' in request.params))
anyway=bool(request.params.get('anyway')), increment_views=bool(request.params.get('increment_views'))


if 'customthumb' in form:
if request.params.get('customthumb', False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Fully preserving the original behaviour (correct):

Suggested change
if request.params.get('customthumb', False):
if 'customthumb' in request.params:

sites = zip(site_names, site_values)

if 'more' in request.params:
form = request.params.mixed()
Copy link
Contributor

Choose a reason for hiding this comment

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

mixed adds a lot of potential type confusion. I think it’s best to create a dict explicitly instead.

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.

3 participants