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

Beat Saber 0.12.0p1 compatibility #8

Merged
merged 13 commits into from
Dec 15, 2018
Merged

Beat Saber 0.12.0p1 compatibility #8

merged 13 commits into from
Dec 15, 2018

Conversation

Zingabopp
Copy link
Contributor

Everything appears to be working for 0.12.0p1. It's the first pull request I've done, so I'm not exactly sure what the best practices are. If you can, you'll probably want to only take the .cs files because somewhere along the line VS2017's Intellisense stopped working (everything still compiles, but it's really annoying).

@opl-
Copy link
Owner

opl- commented Dec 6, 2018

I'll look over it when I have some time; busy week. Thanks!

@opl-
Copy link
Owner

opl- commented Dec 7, 2018

First and most importantly, your editor changed the formatting from tabs to spaces, which makes the diff very difficult to read - even lines you didn't modify are marked as modified. That makes it difficult to tell what was actually changed in your commit and makes it impossible for git to perform some actions on those files. Always make sure your commits only change the lines you actually changed.

Additionally your PR doesn't include the latest changes from the master branch. You effectively reverted the move of websocket-sharp.dll to a different directory (and possibly others? - hard to tell due to indentation changes). Worth noting here is issue #6 which points out that as of commit f22d7b obstacle entering and exiting detection has been broken, which I was unable to create a fix for since then and should be reverted or fixed before the next release.

Lastly, your PR changes relative paths within the project to absolute paths on your system, which will not work for other people. The project uses relative paths with the intention of the correct files either being symlinked from their relative paths to the correct paths or simply copied. Your PR creates the file BeatSaberHTTPStatusPlugin.csproj.user which upon a quick search appears to be the file where such overrides could also take place, which might be worth looking into. Obviously, your commits should not change configuration like this either.

Your commit also changes the .sln file and, admittedly, I'm not sure if those changes affect anything, which is why I'd once again recommend you don't include those changes in your commits, unless someone can explain what the GlobalSection(SolutionProperties) = preSolution and GlobalSection(ExtensibilityGlobals) = postSolution sections do. The latter seems to point to a GUID that doesn't exist anywhere else which makes me believe it could once again be a user specific thing.

Please fix the above issues (especially the one changing the indentation) - I won't be able to merge this PR until those are resolved. I will test the code changes themselves in the next few days when I have access to a Vive.

I hope the above explanations will help you understand how to improve your PRs in the future and thanks again for taking your time to patch the mod for the new release.

@Zingabopp
Copy link
Contributor Author

Thanks for the feedback, I've gotten some things fixed. I'm out of time for now, but I'll look into the other stuff when I can.

@Zingabopp
Copy link
Contributor Author

I Think I've got everything fixed now. Let me know if there's anything else.

@opl- opl- changed the base branch from master to update/0.12 December 15, 2018 20:29
@opl- opl- merged commit 8708e0f into opl-:update/0.12 Dec 15, 2018
@opl-
Copy link
Owner

opl- commented Dec 23, 2018

Retroactively posting this here:

The GameStatus object is supposed to only contain primitive values to make it easy to make a clone of it in case the state needs to be preserved without making it impossible to add new changes to it. I know it's not obvious due to lack of documentation from me, but still letting you know for the future.

rynan4818 pushed a commit to rynan4818/beatsaber-http-status-db that referenced this pull request Oct 9, 2021
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