Skip to content

Conversation

@jrschumacher
Copy link
Contributor

@Fesaa this is a pretty big refactor and I'd understand if you don't want to accept it outright. Happy to make additional changes to get it to a place you'd feel comfortable merging.

Using `map[string]func` is convenient, but its hard to read in Go and equally hard to test.
Copy link
Owner

@Fesaa Fesaa left a comment

Choose a reason for hiding this comment

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

Heya, woah! Big PR, many thanks. Seeing a lot of better design!

I've pulled down, and skimmed over all files and added some comments. If you could resolve these, or respond if appropriate.

Once you've gone over these, I'll go over it all again.

Again, many thanks for the contribution and time you've spent working on this project.

@jrschumacher
Copy link
Contributor Author

Happy New Year @Fesaa! I was able to make all the changes you requested. If you see anything else let me know.

The biggest change is the implementation of the interface for the discord notify service. This will enable you to add other notification services.

@Fesaa
Copy link
Owner

Fesaa commented Jan 3, 2025

Heya!

Have you worked with https://github.com/go-playground/validator before? If so, are you willing to migrate the validation over to it? If not, no worries; I can do it. Same if you don't want to migrate.

Your final commit seems to have broken a test, you'll have to fix this.

I've added my final comments, as well. Lastly, if you could merge master before merging. Thank you!

After that, I think we're ready to get it all merged in 🎉

Thanks for all your work on this update.

~ Amelia

@jrschumacher
Copy link
Contributor Author

Hi! Yes I have used it/something like it. Let's add the validator to the next PR. I plan on doing a couple more, but wanted to get this larger refactor in first.

I fixed the tests by adding TODOs and skipping the failing ones. I'm not sure I understand the logic of the modifiers since they will always iterate the c.currentDate after they process each event (unless the event doesn't have a start date)

Copy link
Owner

@Fesaa Fesaa left a comment

Choose a reason for hiding this comment

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

Two finals comments, I will merge tomorrow and fix them myself if you haven't had the time yet.
All good with the tests, I will have a look at some point.

Thanks

@Fesaa Fesaa merged commit 8463d46 into Fesaa:master Jan 5, 2025
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.

2 participants