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

The rstrip() method removes slashes at the end of the path string instead of slicing #5724

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

geyaning
Copy link
Contributor

@geyaning geyaning commented Jul 13, 2023

The rstrip() method removes slashes at the end of the path string instead of slicing
Move the assignment to self.server.instance_phoned_back inside the conditional statement to avoid unnecessary judgment.

@geyaning geyaning force-pushed the master branch 7 times, most recently from ea97db8 to d0fc507 Compare July 13, 2023 09:50
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @geyaning, thank you for your contribution. I have just a few comments to make tests happy.

PS: I also noticed that you use your master branch for creating PRs. It is a good practice to create a working branch and not use master for development. It will make you live easier and you can work on more than one branches at a time. For more info about PR process, you can read this article.

path = self.path[1:]
if path[-1] == "/":
path = path[:-1]
path = self.path.rstrip('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous code also removed the first character, therefore your changes change the behavior and tests are failing. What about to use self.path[1:].rstrip("/") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your prompt, the access control is OK now, you can check what other requirements, please tell me, if not, please merge

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@richtja richtja merged commit 5fbbe16 into avocado-framework:master Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants