Skip to content

Add /config reset#28

Open
sh0ckR6 wants to merge 6 commits intosh0ckdotlive:masterfrom
sh0ckR6:issues/16-config-reset
Open

Add /config reset#28
sh0ckR6 wants to merge 6 commits intosh0ckdotlive:masterfrom
sh0ckR6:issues/16-config-reset

Conversation

@sh0ckR6
Copy link
Copy Markdown
Member

@sh0ckR6 sh0ckR6 commented Nov 21, 2021

Add support for resetting configuration files via the /config command.


This repo hasn't been worked on for quite some time. I wanted to give this project the love it deserves, so I came back to it after a couple months of letting it sit.

@sh0ckR6 sh0ckR6 requested a review from gtaEPIC November 21, 2021 06:03
@sh0ckR6 sh0ckR6 added priority: medium This needs to be completed soon status: in progress This is still a work in progress type: feature This is related to a feature labels Nov 21, 2021
@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

and all of my commits are unverified...

might force push and fix that

@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

@gtaEPIC If you don't mind reviewing this PR even though it's been a hot minute, I'd really appreciate it! If not, I can review the PR on my own in the morning and merge it in anyways.

I haven't really tested it as it's late, so for all I know it could be hella broken. Look about right though, didn't seem too hard to add.

@gtaEPIC
Copy link
Copy Markdown
Contributor

gtaEPIC commented Nov 21, 2021

Code looks good, I'll test it in the morning to make sure it works before submitting a review.

@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

Sounds good! I'll force push with signed commits because I don't like seeing unverified. Hopefully I can get those up tonight. If not, I wouldn't recommend testing until I force push the signed commits, just in case anything went wrong with the force push :D

@sh0ckR6 sh0ckR6 force-pushed the issues/16-config-reset branch from cf1f675 to 5572867 Compare November 21, 2021 07:37
Added support for resetting s back to their defaults via
Add reloading/resetting configuration by  object
Mark methods `static` that needed to be static
Support config resetting
@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

of course I forgot a commit, let me try force pushing for a second time

@sh0ckR6 sh0ckR6 force-pushed the issues/16-config-reset branch from 5572867 to 0cc50be Compare November 21, 2021 07:38
@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

there we are, all 5 commits signed!

off to bed I go, this has been a confusing and chaotic past 2 hours -_-

Copy link
Copy Markdown
Contributor

@gtaEPIC gtaEPIC left a comment

Choose a reason for hiding this comment

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

An exception is thrown if no arguments are provided

@gtaEPIC
Copy link
Copy Markdown
Contributor

gtaEPIC commented Nov 21, 2021

Also, not quite sure if it was intended, but if you load a new map, the configurations need to be reset in order to have the logs spawn under the player when they join

@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

Yeah, I've noticed that when moving between the test world and my main world. Either you or I can open an issue for this as it's definitely not intentional.

Add argument length checks for `handleReload` and `handleReset`
@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

Updated PR with requested changes

@sh0ckR6 sh0ckR6 requested a review from gtaEPIC November 21, 2021 16:37
Copy link
Copy Markdown
Contributor

@gtaEPIC gtaEPIC left a comment

Choose a reason for hiding this comment

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

Seems good now!

@sh0ckR6
Copy link
Copy Markdown
Member Author

sh0ckR6 commented Nov 21, 2021

So turns out, this PR is completely broken. Configuration resetting just doesn't work at all (which is quite infuriating but that's fine)

I'll have to look into why this isn't working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: medium This needs to be completed soon status: in progress This is still a work in progress type: feature This is related to a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] - /config reset Command [FEATURE] - In-game Config Management

2 participants