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

Update config file location / implement set command #72

Merged
merged 2 commits into from Aug 15, 2019

Conversation

sambattalio
Copy link
Collaborator

Description

I implemented the :set command and switched the config file location to be /TerminusBrowser instead of /commandChan.

For the :set command, it works with either:

:set KEY VALUE

or

:set SITE KEY VALUE (ex. :set REDDIT username test)

The config currently stores the enum values of the sites as opposed to the "normal" name. What do you guys think about this? Should I open another PR possibly

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the style of this project
  • I have performed a self-review of my own code
  • My code is self documenting
  • I have made corresponding changes to the documentation
  • My changes generate no new major crashes/bugs

@sambattalio
Copy link
Collaborator Author

updating test rn

Copy link
Collaborator

@ginglis13 ginglis13 left a comment

Choose a reason for hiding this comment

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

LGTM. Is it expected that a user can set any key they want in the config file? this is nbd but just asking, for future it might be cool to somehow tell the user if something they've set is a valid setting or not.

@sambattalio
Copy link
Collaborator Author

sambattalio commented Aug 14, 2019

Yea I mean right now it can just add anything afaik thats a good thought though. Right now, the set doesn't have too to much use but it should be more valuable as things like username are added.

@wtheisen
Copy link
Owner

This seems more like an mvp feature set right now however as long as it doesn't break any current functionality I am a big fan of pulling in MVP work!!

@wtheisen wtheisen merged commit 80ad3ee into wtheisen:master Aug 15, 2019
@sambattalio
Copy link
Collaborator Author

I really feel like you're the MVP ❤️ @wtheisen

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.

None yet

3 participants