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

Move laargs.txt to more suitable location. #83

Merged
merged 15 commits into from
Dec 12, 2023
Merged

Move laargs.txt to more suitable location. #83

merged 15 commits into from
Dec 12, 2023

Conversation

iamalexrouse
Copy link
Contributor

@iamalexrouse iamalexrouse commented Dec 7, 2023

This moves the laargs.txt file from the root directory of the application. (Likely in a protected folder.) to a user based one. Allowing different users to activate different settings.

Fixes #82.

Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
@zabszk zabszk self-assigned this Dec 7, 2023
@zabszk
Copy link
Member

zabszk commented Dec 7, 2023

Additionally, we should consider implementing migration of the file to the new path, so people who use the file currently can just update LA to have it working. If possible, we shouldn't introduce changing requiring manual changes.

In my opinion it should work like this:

  1. If the file exists in the new directory - do nothing.
  2. If it doesn't and there is a non-empty file in the old directory - move it to the new directory.
  3. Regardless of the above, if there is an empty file in the old directory - delete it.

@zabszk zabszk linked an issue Dec 7, 2023 that may be closed by this pull request
@zabszk
Copy link
Member

zabszk commented Dec 7, 2023

Btw. if you use keywords such as "Closes #82", "Fixes #82", "Resolves #82", etc... in the PR description, the issue will be automatically linked to this PR and closed when merging.

@iamalexrouse
Copy link
Contributor Author

Additionally, we should consider implementing migration of the file to the new path, so people who use the file currently can just update LA to have it working. If possible, we shouldn't introduce changing requiring manual changes.

In my opinion it should work like this:

  1. If the file exists in the new directory - do nothing.
  2. If it doesn't and there is a non-empty file in the old directory - move it to the new directory.
  3. Regardless of the above, if there is an empty file in the old directory - delete it.

Ok, I'll implement a migration system.

@iamalexrouse
Copy link
Contributor Author

image
In progress report.

@iamalexrouse iamalexrouse requested a review from zabszk December 7, 2023 15:26
Core/StartupArgManager.cs Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
@iamalexrouse
Copy link
Contributor Author

Testing...

@iamalexrouse iamalexrouse requested a review from zabszk December 8, 2023 06:11
Core/Program.cs Outdated Show resolved Hide resolved
Core/Program.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
@iamalexrouse iamalexrouse requested a review from zabszk December 9, 2023 04:41
Copy link
Member

@zabszk zabszk left a comment

Choose a reason for hiding this comment

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

It completely doesn't follow the logic we want (#83 (comment)).

  1. Your code can delete old file without moving it. We can't delete someone's config. Even if there is a new one and we don't use the old one, we can't delete it (if it contains some data). Maybe the user put something important there.
  2. Previous version of LA was always creating an empty file and we don't want to move it, we want to get rid of it. Now there is no check for that after your latest changes.

@iamalexrouse
Copy link
Contributor Author

I'm going to just do a full rewrite tomorrow.

@iamalexrouse
Copy link
Contributor Author

It completely doesn't follow the logic we want (#83 (comment)).

  1. Your code can delete old file without moving it. We can't delete someone's config. Even if there is a new one and we don't use the old one, we can't delete it (if it contains some data). Maybe the user put something important there.
  2. Previous version of LA was always creating an empty file and we don't want to move it, we want to get rid of it. Now there is no check for that after your latest changes.

Could I get a bit of clarification on this?

@zabszk
Copy link
Member

zabszk commented Dec 9, 2023

It completely doesn't follow the logic we want (#83 (comment)).

  1. Your code can delete old file without moving it. We can't delete someone's config. Even if there is a new one and we don't use the old one, we can't delete it (if it contains some data). Maybe the user put something important there.
  2. Previous version of LA was always creating an empty file and we don't want to move it, we want to get rid of it. Now there is no check for that after your latest changes.

Could I get a bit of clarification on this?

The migration process should look like this:

  1. If the old file exists and is empty/contains only white space -> delete that file, migration process ends.
  2. If the new file exists -> abort migration at this stage.
  3. Otherwise, move the old file to the new location.

Currently code in this PR does completely something else:

  1. If the new file doesn't exist and old file exists -> move old file (without checking if it's even being used or if it's empty or not).
  2. Always deletes the old file (even if it contains data and is not being moved to the new location).

And in a situation when new file exists and the old doesn't your code throws an exception (that is handled by the try-catch block). And that situation persists, so it's not one-time issue. It will happen on every LA start.

@iamalexrouse
Copy link
Contributor Author

Right, thanks. I'll remake the migrator based on this.

Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
Core/StartupArgManager.cs Outdated Show resolved Hide resolved
@iamalexrouse iamalexrouse requested a review from zabszk December 12, 2023 14:53
@zabszk zabszk merged commit f6e03f6 into northwood-studios:master Dec 12, 2023
2 checks passed
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.

Move laargs.txt to AppData.
2 participants