Skip to content

Conversation

@zfi
Copy link

@zfi zfi commented Feb 14, 2017

This update add support for placing the event disk log in the user's AppData/Local/Parallax folder or in the user's home directory if the first directory is not available.
Refactored verify_logfile and create_logfile.

Refactored verify_logfile and create_logfile.
@PropGit
Copy link

PropGit commented Feb 14, 2017

Nice work, @zfi !

@PropGit PropGit merged commit 9cff8f7 into parallaxinc:demo Feb 15, 2017
@PropGit
Copy link

PropGit commented Feb 15, 2017

@zfi - Tested but it failed. I'll point out the problem in code review.

Copy link

@PropGit PropGit left a comment

Choose a reason for hiding this comment

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

Log path is in wrong location: ~/AppData/Parallax/ instead of ~/AppData/Local/Parallax/.

(I just learned that I'm supposed to click "Finish Your Review" so this stuff is visible to others too.)

def __verify_macos_logpath(file_path):
def __set_windows_logpath(filename):
user_home = os.path.expanduser('~')
log_path = user_home + '/AppData/Parallax'
Copy link

Choose a reason for hiding this comment

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

@zfi - I failed to notice this when scanning the code, but I see the effects of it in my build and test... It accidentally creates the Parallax subfolder inside the /AppData folder instead of inside the /AppData/Local folder.

image

When run, the /AppData/Parallax folder is empty (no log file appeared). I don't know if this is because the system somehow let the app create the folder but won't let it populate it, or not.

Copy link

@PropGit PropGit Feb 15, 2017

Choose a reason for hiding this comment

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

NOTE: Retesting shows that the Log File field is blank. So, the plan b to store it in the user's home folder didn't work either?

image

@zfi zfi deleted the 170214-WindowsLogging branch February 15, 2017 19:29
@PropGit
Copy link

PropGit commented Feb 16, 2017

Retested with PR #49 and PR #51 - verified works!

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.

3 participants