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

Config files restructure #20

Closed
wants to merge 12 commits into from
Closed

Config files restructure #20

wants to merge 12 commits into from

Conversation

samuelallan72
Copy link

The changes here mostly deal with restructuring how/where confile files, scripts, maps, etc. are loaded from.

  • By default, everything is expected to be under ~/.pysnip (the location can be changed through program arguments)
  • Everything in the configs directory follows a neat structure (as detailed in configs/README.md)
  • I've added a requirements.txt which can be used for installing pip dependencies easily - something could be added to the readme about that (more useful after adding a working setup.py - as in Pysnip as a package #16 which may need modification after these refactorings)
  • A todo: information about config structure should be added to the readme when finalized how it all should work. A script could also be made to copy example config to ~/.pysnip for getting started.


[![Build Status](https://travis-ci.org/NateShoffner/PySnip.svg?branch=master)](https://travis-ci.org/NateShoffner/PySnip)
# About this fork #
Copy link
Author

@samuelallan72 samuelallan72 Apr 1, 2016

Choose a reason for hiding this comment

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

Don't merge in the 'about this fork' part of the readme. EDIT: removed

@noway
Copy link
Contributor

noway commented Dec 28, 2016

This is a great and huge change. I don't really remember the project structure too well to judge whether it correlates with the potential pysnip-as-a-module PR or not.

One concern I have is does it break potentially any userscripts? Feels like feature_server might be specifically expected to be present by certain scripts..

@samuelallan72
Copy link
Author

It's been a while since I looked at this so I could be wrong, but from memory this doesn't change anything in regards to installing as a package. I think once I got the config file parts working I did look into it but got stuck - enet is included with it and I couldn't get it to run properly with the enet from upstream (i was attempting to split it out). I dare say it could be included fine with it to be distributed as a package - I'll have to attack it again later.

@noway421 what exactly do you mean by feature_server expected to be present? All of that should be still in the same place.

Also I'd be interested to know if anyone with commit privileges would be willing to review and merge (or give feedback on) these changes. I'll also fix the readme changes that aren't relevant to this.

Samuel Walladge added 2 commits December 28, 2016 21:05
@noway
Copy link
Contributor

noway commented Dec 29, 2016

Yep, most probably it is not related to pysnip-as-a-module. So in that terms it is something that won't interfere with it. I'd say it is good to merge. Hopefully Nate will take a look into this PR and approve.

I remember I spot some issue with enet too, a better way would be to include it altogether, at least for a first release. I'm seeing no-enet branch in your https://github.com/swalladge/PySnip repo. I'm assuming this is the change you attempted. Good job on that.

I feel like some user scripts (which are in feature_server/scripts/) might expect config.txt and maps/*.txtc in their parent directory i.e. in feature_server. They'll go one directory up from the __file__ and look for them. To be bullet-proof, and while it is not tested by wide audience, for now it would make sense to make symlinks for feature_server/data/, feature_server/config.txt and feature_server/maps/*.txtc. Although the number of scripts which rely on this behaviour is not great most likely. I can only think about the free build script which allows you to save the map with a command sent by a player.

I'm planning to take a deeper look into pip module and I'll try to base off my changes from your config changes (i.e. from https://github.com/swalladge/PySnip master)

@samuelallan72
Copy link
Author

I think another good option would be to inspect all the scripts we currently have and update any references to suit the new config layout. I won't have much time over the next few weeks, but after that I'll check it out.

@noway
Copy link
Contributor

noway commented Dec 30, 2016

I went through your changes again, actually my concerns are unfounded: the userscripts are still relatively the same to maps and config files. They are both in configs while initially I thought they stayed in feature_server with other pyspades core scripts (by those I mean commands.py and such).

In that case, even if some problems do arise it is possible to fix them within the pysnip repo.

👍 I'd say it's good to merge

@StripedMonkey
Copy link

As a legitimate thing who approves the merges and stuff? This has been sitting here for quite a while, I'm looking into OpenSpades development and am looking at this and the client and am rather dissapointed in how lackluster the activity is.

@samuelallan72
Copy link
Author

@StripedMonkey development on PySnip hasn't happened for a long time unfortunately. You might want to check out an active fork that includes this PR and many new improvements: https://github.com/piqueserver/piqueserver/ :)

@StripedMonkey
Copy link

Cool, I suppose part of the issue is that this is the server management that is advertised on https://www.buildandshoot.com/download/ I didn't see the other one.

@noway
Copy link
Contributor

noway commented Mar 8, 2018

BNS link should really be updated tbh

@samuelallan72
Copy link
Author

samuelallan72 commented Mar 8, 2018

@StripedMonkey @noway see pr --> BuildandShoot/buildandshoot.github.io#12 feedback welcome! Hopefully a maintainer there will be willing to merge. 😄

Edit: though this isn't the place to discuss this - jump on one of the bridged piqueserver chat channels!

@samuelallan72
Copy link
Author

Closing this pull request since everything useful in here has been merged to piqueserver and I have no intention to work on or maintain anything in this PR/repo in the future.

@NotAFile
Copy link

RIP pull-request, you served us well

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.

4 participants