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

Bot Setups feature relates to #94 #115

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

skliffmueller
Copy link

@skliffmueller skliffmueller commented Mar 14, 2019

Fully tested and ready.

@skliffmueller skliffmueller changed the title Bot Setups feature relatets to #94 Bot Setups feature relates to #94 Mar 14, 2019
@skliffmueller
Copy link
Author

Getting it to build, didn't have docker handy at the time. Damn ESEA blocking virtualization. So had to spool up a docker on a remote host and get a server running. Will post results.

…s to

have an optional argument .loadbots <myprofile> .savebots <myprofile>
while still being backwards compatable with original .loadbots .savebots
commands

Updated README to include bot setups, tabs vs spaces

Bot Setups: Fixed build issues

Bot Setups: Fixed build issues

Stuff

Fixing more variable issues

Fixing stuff
.removebotsetup <setup>)

Updated README to include bot setups, tabs vs spaces

Fixed sizeof reference

Fixed symlink

Stuff

Fixing more variable issues

Fixing stuff

debug message

Debugging

debug stuff

More debugging

stuff

Fixed compile error

bla

asd

asdasd

bug fixes

strlen not sizeof

debug

print stuff

fix

list fix

tweaks

more bug fixes

stuff

Finished
@skliffmueller
Copy link
Author

It's done and tested. I added .removebotsetup as I can imagine over time people having too many configs.

@splewis
Copy link
Owner

splewis commented Mar 17, 2019

Thanks for taking a shot at this. I'll try to take a further look soon.

One thing from a quick pass: it looks like you're using a separate file for each setup, which gets named by the user. I think it'd be strongly preferable to continue using a single file for all bot-info on each file. The keyvalues structure allows for things to be nested.

It's a little awkward because of the existing saving/loads of named bot positions. I'm not sure how that should interplay with this new feature, or if that should even exist at all. It didn't really turn out to work very well so I'd be okay if it was removed.

And on a slightly more trivial note: keeping the style in line with all the code would be nice too. From a quick glance, I see some inconsistencies from the existing code. The existing code uses if (foo) over if(foo) (and for loops as well). I'm basically just following https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace.

@skliffmueller
Copy link
Author

skliffmueller commented Mar 18, 2019

I will improve on this soon. With the work week, improvements will be delayed.

Another thought. For a legacy fallback. The old data format can be detected on load, and transformed into the new KeyValue object format using the "default" key, and save would just save into the new format overwriting the file.

@danieliser
Copy link

@skliffmueller Any luck with the changes? This looks like a great addition too.

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