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

Fixed changing location for show. #5795

Merged
merged 5 commits into from
Jan 12, 2019
Merged

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Nov 28, 2018

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

@ghost
Copy link

ghost commented Nov 28, 2018

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

@@ -1681,6 +1716,7 @@ def delete_show(self, full=False):
# remove entire show folder
if full:
try:
_ = self.validate_location # Let's get the exception out of the way asap.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the need of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.location does now by default all kind of checks on the filesystem. So if we use it in the apiv2, it will also do those checks there. In stead of having the option to just get the self.location (without any checks) and one optional that does the validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ow wait, this line.. Pff i have to dig for that

@@ -1681,6 +1716,7 @@ def delete_show(self, full=False):
# remove entire show folder
if full:
try:
_ = self.validate_location # Let's get the exception out of the way asap.
log.info(u'{id}: Attempt to {action} show folder {location}',
Copy link
Contributor Author

@p0psicles p0psicles Jan 12, 2019

Choose a reason for hiding this comment

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

Okay, see how here self.location was included in the log. Which previously meant, that this could raise an exception. Just from the info log.

link 1701 self.location was also used. Another place where the exception could have been raised. self.location is now (what was previously self.raw_location), i'm using self.location where it makes sense. But to keep the possibiliy the exception is raised, i put it in line 1719

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you are going for now.

@ghost
Copy link

ghost commented Jan 12, 2019

DeepCode analyzed this pull request.
There are no new issues.

@medariox medariox merged commit 017a9c7 into develop Jan 12, 2019
@medariox medariox deleted the feature/fix-setting-location branch January 12, 2019 13:21
@sharkykh sharkykh added this to the 0.3.0 milestone Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants