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

fix script_name without / at beginning #1835

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

Conversation

abtinmo
Copy link

@abtinmo abtinmo commented Dec 28, 2019

What kind of change does this PR introduce?

  • bug fix
  • feature
  • docs update
  • tests/coverage improvement
  • refactoring
  • other

What is the related issue number (starting with #)

#1772

What is the current behavior? (You can also link to an open issue here)

#1772

What is the new behavior (if this is a feature change)?

  1. shows an error, telling: script_name foo should start with /

  2. fixes the script_name to prevent 404 error (not sure if it's necessary)

Other information:

Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

@webknjaz
Copy link
Member

webknjaz commented Dec 29, 2019

Hey @abtinmo, thanks for the PR!

Please add a test for this change. And make sure the CI is green.

@codecov
Copy link

codecov bot commented Dec 29, 2019

Codecov Report

Merging #1835 into master will decrease coverage by 2.89%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1835     +/-   ##
=========================================
- Coverage   81.21%   78.31%   -2.9%     
=========================================
  Files         104      104             
  Lines       13625    13264    -361     
=========================================
- Hits        11065    10388    -677     
- Misses       2560     2876    +316

@abtinmo
Copy link
Author

abtinmo commented Dec 29, 2019

Hi @webknjaz,

it seems there is a problem with your macos config for ci.
and failed with AppVeyor is for #1750

@webknjaz
Copy link
Member

@abtinmo don't worry Circle CI and that flaky test. Just add a test case for your change.

@abtinmo
Copy link
Author

abtinmo commented Dec 30, 2019

@webknjaz its done.

@webknjaz
Copy link
Member

This change feels dangerous so I would also require a clear approval from @jaraco, not just myself.

cherrypy.log.error(
msg='script_name {0} should start with /'.format(script_name)
)
script_name = '/' + script_name
Copy link
Member

Choose a reason for hiding this comment

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

I'm disinclined to continue to let the user supply an erroneous value and correct it for them. Why not instead hard-fail (raise an exception) when invalid values are input (similar to how script_name=None is handled)?

Copy link
Author

Choose a reason for hiding this comment

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

fixes the script_name to prevent 404 error (not sure if it's necessary) , its what i said in my first comment.
i don't think letting user to continue is a right solution. but in doc i saw this:
This should start with a slash

so i choose to fix error myself and throw a console error.
raise an exception is indeed a better solution

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

AFAIR we discussed raising an exception for the invalid input. I'll wait for this change before attempting to review again.

@jaraco jaraco changed the base branch from master to main March 23, 2021 00:13
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

3 participants