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

Rewrite and implement }karmalink & }karmaunlink #9

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

dsprenkels
Copy link

@dsprenkels dsprenkels commented Nov 8, 2020

Changes in this commit:

  • Update all the dependencies to recent versions.
  • Update the Rust syntax to 2018 edition.
  • Rewrite the parser using nom.
  • Rewrite the JSON (de)serializing using serde.
  • Implement karma groups.
  • Implement that linking and unlinking of groups
    using the }karmalink and }karmaunlink commands.

This patch rewrites the whole plugin almost completely. The reason that this
was necessary was because some of the previous dependencies changed so much
over the years, that their usage would have to be rewritten from scratch
anyway. In particular in the case of peg and rustc_serialize, we have much
better crate support nowadays, so I replaced them with their modern
substitutes (nom and serde).

Fixes #5
Fixes #8

@dsprenkels dsprenkels force-pushed the karmalink branch 2 times, most recently from 4ab4baf to c44e3cc Compare November 8, 2020 20:44
Copy link

@thomwiggers thomwiggers left a comment

Choose a reason for hiding this comment

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

Some small things.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
[dependencies.chrono]
version = "0.2"
[profile.release]
overflow-checks = true

Choose a reason for hiding this comment

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

The overhead is negligible, but why?

Copy link
Author

Choose a reason for hiding this comment

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

Because the overhead is negligible and I hate overflow bugs. :P

Feel free to veto me on this.

src/karma.rs Outdated Show resolved Hide resolved
src/handler.rs Outdated Show resolved Hide resolved
src/handler.rs Outdated Show resolved Hide resolved
Copy link

@Lucus16 Lucus16 left a comment

Choose a reason for hiding this comment

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

Most logic and storage interaction is in karma.rs including the logic for computing the group from an initial karma, but not the logic for linking karma. That logic depends on each other for correctness though. I think it makes sense to put all logic and storage interaction in karma.rs and only the human interface in handler.rs.

src/parse.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
src/karma.rs Outdated Show resolved Hide resolved
dsprenkels and others added 2 commits November 17, 2020 12:52
@dsprenkels
Copy link
Author

Most logic and storage interaction is in karma.rs including the logic for computing the group from an initial karma, but not the logic for linking karma. That logic depends on each other for correctness though. I think it makes sense to put all logic and storage interaction in karma.rs and only the human interface in handler.rs.

I will take a look at this when applying the fixes. I'm unsure whether I'll fix this though. In any case I'd like to prevent karma.rs from becoming a god module.

@dsprenkels
Copy link
Author

Btw. Perhaps I will take a look at that after merging this change. :P

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.

Add support for karma-aliasing
3 participants