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.

Sign up for GitHub

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

Jump to bottom

reorg #40

Merged
merged 5 commits into from
Nov 8, 2019
Merged

reorg #40

merged 5 commits into from
Nov 8, 2019

Conversation

mmou
Copy link
Contributor

@mmou mmou commented Nov 8, 2019 •
edited
Loading

messy commit history...but all this is doing is

  • moving some tests from kbchat_test.go to chat_test.go, if the function it's testing was moved to chat.go
  • moving structs from types.go that are only used in 1 other go file into that go file

#39 builds on this PR

@mmou mmou requested review from joshblum and xgess November 8, 2019 08:26
joshblum
joshblum approved these changes Nov 8, 2019
View reviewed changes
Copy link
Member

@joshblum joshblum left a comment

Choose a reason for hiding this comment

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

lgtm for refactor. what is the purpose of the example renames? prefer without the number prefix.

@joshblum joshblum mentioned this pull request Nov 8, 2019
Merged
@mmou
Copy link
Contributor Author

mmou commented Nov 8, 2019

@joshblum the example renames were to make them match the examples in the python bot (@xgess); the number prefix gives a suggested "difficulty/complexity" of the bot example. i don't feel strongly about it either way

@mmou mmou merged commit 10c76c4 into master Nov 8, 2019
@mmou mmou deleted the mmou-reorg2 branch November 8, 2019 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblum joshblum joshblum approved these changes

@xgess xgess Awaiting requested review from xgess

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mmou @joshblum