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

refactor: Used urljoin where + is used #2161

Closed
wants to merge 2 commits into from
Closed

Conversation

Beeram12
Copy link
Contributor

@Beeram12 Beeram12 commented Jan 18, 2024

Used urljoin instead of + to resolve error

Fixes

@ReimarBauer
Copy link
Member

ReimarBauer commented Jan 20, 2024

Maybe use branch names based on develop by e.g. issueNumber
Usually one can close a branch when the PR is merged

@@ -955,7 +955,8 @@ def get_recent_op_id(self):
"token": self.token,
"skip_archived": skip_archived
}
r = requests.get(self.mscolab_server_url + '/operations', data=data)
url = urljoin(self.mscolab_server_url, '/operations')
r = requests.get(url, data=data)
Copy link
Member

Choose a reason for hiding this comment

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

have a look on the docs of urljoin

r = requests.get(self.mscolab_server_url + '/operations', data=data,
timeout=tuple(config_loader(dataset="MSCOLAB_timeout")))
url = urljoin(self.mscolab_server_url, '/operations')
timeout = tuple(config_loader(dataset="MSCOLAB_timeout"))
Copy link
Member

Choose a reason for hiding this comment

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

don't introduce new names, when it is not needed somewhere else

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

also look in the failing tests related to your change

@ReimarBauer
Copy link
Member

@Beeram12 do you want to finish this?

@Beeram12 Beeram12 closed this Feb 16, 2024
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.

2 participants