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

Highlights #29

Merged
merged 27 commits into from Apr 14, 2021
Merged

Highlights #29

merged 27 commits into from Apr 14, 2021

Conversation

legendofmiracles
Copy link
Contributor

@legendofmiracles legendofmiracles commented Mar 25, 2021 •
edited

This PR implements #22
Missing features/what should still be done:

  • more testing
  • more testing
  • more testing
  • did I mention testing?
  • performance testing

elkowar
elkowar requested changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@elkowar elkowar left a comment

Choose a reason for hiding this comment

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

uwu

src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/db/highlights.rs Outdated Show resolved Hide resolved
src/events/message.rs Outdated Show resolved Hide resolved
src/events/message.rs Outdated Show resolved Hide resolved
src/db/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
@legendofmiracles legendofmiracles marked this pull request as ready for review March 27, 2021 14:31
@legendofmiracles
Copy link
Contributor Author

will this be merged?

@elkowar
Copy link
Contributor

elkowar commented Apr 5, 2021

Haven't tested it all that thoroughly yet, but if eversthing works then probably!

elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/db/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/events/message.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/events/message.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/events/message.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/db/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar reviewed Apr 6, 2021
View reviewed changes
src/db/highlights.rs Outdated Show resolved Hide resolved
elkowar
elkowar requested changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@elkowar elkowar left a comment

Choose a reason for hiding this comment

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

still some stuff uwu

@legendofmiracles
Copy link
Contributor Author

Stupid cargo-fmt

elkowar
elkowar requested changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@elkowar elkowar left a comment

Choose a reason for hiding this comment

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

some more minor stuff, plus the one function i sent in discord uwu

src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
src/commands/highlights.rs Outdated Show resolved Hide resolved
@elkowar
Copy link
Contributor

elkowar commented Apr 8, 2021

Additionally, as discussed in discord:

I think it may make sense to send a DM to users when they set up their first highlight, to check if they have DMs open - otherwise users may be confused if it doesn't work and they have their DMs closed. we should then give an error message when the DM fails, and only add the highlight if it succeeded

@elkowar
Copy link
Contributor

elkowar commented Apr 8, 2021

Also, as discussed, i think calling the command highlight rather than highlights might be preferable, we can add an alias to make both work. for get i'd also add a list alias, as my intuition would be that it's !highlight list

@legendofmiracles
Copy link
Contributor Author

My clippy doesn't fricking report the stupid error, except when building in relase mode...

elkowar
elkowar reviewed Apr 12, 2021
View reviewed changes
src/commands/highlights.rs Outdated Show resolved Hide resolved
@elkowar elkowar merged commit c575cd1 into unixporn:master Apr 14, 2021
@elkowar
Copy link
Contributor

elkowar commented Apr 14, 2021

Very poggers, my dude! Thanks a ton!

@elkowar elkowar mentioned this pull request Apr 14, 2021
Closed
@legendofmiracles
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elkowar elkowar elkowar requested changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@legendofmiracles @elkowar