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 Issue 624 #631

Merged
merged 1 commit into from
Apr 12, 2017
Merged

Fix Issue 624 #631

merged 1 commit into from
Apr 12, 2017

Conversation

jipumarino
Copy link
Contributor

@jipumarino jipumarino commented Apr 6, 2017

self.file_path and self.repo_path are None when view is cached.

Fixes #624

Copy link
Member

@stoivo stoivo left a comment

Choose a reason for hiding this comment

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

It work like it should. Thanks for your contribution. Till next time, your commit message should me something more descriptive

Improve: Show open inline diff if it already open form statu dashboard

fix 624

I can change the commit message

I will leave it open 2 days for discussion, if wanted.

@jipumarino
Copy link
Contributor Author

Thank you @stoivo, this was my first time using the "edit" feature from github, I didn't realize I was writing a commit message instead of the pull request title until it was done; I'll appreciate it if you change it.

I'm not sure yours is correct though, this feature was already implemented, I was just fixing an error introduced in 77471fd#diff-1d123c94cf2d78df2c8edace6fa6ee97R70

@stoivo
Copy link
Member

stoivo commented Apr 7, 2017

I just glared at the code. I don't understand why my path 77461fd broke this.

@jipumarino
Copy link
Contributor Author

Because of what I commented before. When the diff view is already open both self.repo_path and self.file_path are set to None at that point (not sure why that happens though).

When it's not open, those properties get set in this loop: 77471fd#diff-1d123c94cf2d78df2c8edace6fa6ee97L65

I tried replicating that loop in the other if branch, but it didn't work. This was the most straightforward solution.

@stoivo stoivo merged commit 8848cb6 into timbrel:master Apr 12, 2017
@stoivo
Copy link
Member

stoivo commented Apr 12, 2017

Thanks again

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

2 participants