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

Add optional -g/--grimoire flag to choose source file other than Niet… #131

Merged
merged 4 commits into from
Apr 14, 2024

Conversation

ginger51011
Copy link
Collaborator

…zche

While I do think all bots enjoy Nietzche (who doesn't?), I think we should take a stance to educate them. What better way than to be able to choose from any book!

Personal suggestions include:

  • The Sorrows of Young Werther by Goethe
  • Any political manifesto
  • The Declaration of Independence

etc. etc.

I tried to fit this feature in without doing a refactor, but perhaps my way of doing this is disturbing in other ways than the implicated use case.

…zche

While I do think all bots enjoy Nietzche (who doesn't?), I think we should
take a stance to educate them. What better way than to be able to choose
from any book!

Personal suggestions include:

- The Sorrows of Young Werther by Goethe
- Any political manifesto
- The Declaration of Independence

etc. etc.
@yunginnanet yunginnanet self-assigned this Jan 19, 2024
heffalump/heffalump.go Outdated Show resolved Hide resolved
heffalump/heffalump.go Outdated Show resolved Hide resolved
This removes globals from `heffalumpt/`, which are hard to reason about,
easy to get wrong, and should not be created if they are never used.

A possible drawbacks is if you would create multiple new defaults,
but this should never be the case.
@ginger51011
Copy link
Collaborator Author

See this commit message for rationale about removing globals and markovmap init().

This prints the short variants (like `-c` for `--config`)
in the help.

Also fixes bug where only `-h` flag works, not `--help`.
internal/config/config.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internal/config/arguments.go Show resolved Hide resolved
@@ -18,11 +18,12 @@ var CLI = help{
title: Title,
version: Version,
usage: map[int][]string{
0: {0: "--config", 1: "<file>", 2: "Specify config file"},
0: {0: "-c, --config", 1: "<file>", 2: "Specify config file"},
Copy link
Owner

Choose a reason for hiding this comment

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

does this still end up being aligned properly when you run --help?

i consider my code here for the CLI alignment to be pretty janky and it may be sensitive to this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked and it's aligned in a way 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

hmm yeah, i took a look - definitely busted with this change

i don't consider your change to be at fault here, I blame my janktastic code that surrounds it

with that being said; we'll have to refactor the help cli somehow to account for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a look, but as you said, that can of worms would be a nice use of the new flag (no offense). Might need a refactor (or perhaps outsource the whole CLI thing to a dependency)

Copy link
Owner

Choose a reason for hiding this comment

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

i'd like to avoid 3rd party dependencies, but stdlib flags package would probably work just fine

to tell you the truth i never expected this much involvement (incredibly grateful to see it though), or else I wouldn't have set contributors up for failure in this context - so sorry about that!

if i have time i'll try to look into this, but if you just hack the thing to bits and replace it, I won't be at all offended

you can expect my usual annoying gopher style feedback, or I may collab with you and modify it - but that's not personal at all and I'll always accept feedback in the other direction :D

but with that in mind; go ahead and rip that shit up if you find the time before I do (open invitation to any contributor)

internal/http/router.go Outdated Show resolved Hide resolved
@yunginnanet yunginnanet removed their assignment Feb 1, 2024
@ginger51011
Copy link
Collaborator Author

Unrelated to this PR: Thanks for the shoutout in the README 😄

README.md Show resolved Hide resolved
@yunginnanet
Copy link
Owner

yunginnanet commented Feb 2, 2024

something else to keep in mind would be if there are any conflicts with #137 -
these two PRs touch on very similar, and in both cases fundamental, mechanics

i'm also starting to think that due to the lack of unit tests ( #139 ) along with the way both PRs change fundamental mechanics - it may be wise for me to bring development branch up to date and for all relevant parties to consider changing the base of both PRs to development

this would allow us to move forward with both merges with more confidence that we aren't going to FUBAR main on accident

fixing #139 of course would be an alternate solution - broken record here, but I'll try to find some time to address these matters soon

@ginger51011 ginger51011 changed the title Add optional -b/--book flag to choose source file other than Niet… Add optional -g/--grimoire flag to choose source file other than Niet… Feb 5, 2024
@yunginnanet
Copy link
Owner

i'd say just drop the aesthetically breaking changes in the cli part of the code for now (people can read the docs and wait for us to refactor that) so we can get this merged

@yunginnanet yunginnanet changed the base branch from main to development April 14, 2024 23:53
@yunginnanet yunginnanet merged commit 6e183db into yunginnanet:development Apr 14, 2024
1 check passed
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

2 participants