Strict toml marshalling

[randomdude16671]

This prevents users from putting in invalid keys. I also made it print details of the error if the error doesn't result to nil and same with the details. This was done by not using the toml.Unmarshal alias and creating a Decoder manually with a strings.reader(file)
as the argument and enabling strict marshalling to the Decoder,

[Copilot]

Pull Request Overview

This PR introduces strict TOML marshalling to prevent users from providing invalid keys and improves error reporting by printing detailed error information. Key changes include updating the signature of GetConfig to return error details, replacing toml.Unmarshal with a manually configured Decoder using DisallowUnknownFields, and altering error handling logic in seshcli/seshcli.go to print error details when available.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
seshcli/seshcli.go	Updates error handling to accommodate new GetConfig signature changes.
configurator/configurator.go	Implements strict TOML decoding with detailed error reporting and modifies function signatures accordingly.

[randomdude16671]

It seems ok to me. My apologies for the errors, this is my firet time sending a PR

[randomdude16671]

My apologies for the errors, its first time sending a PR

[joshmedeski]

I tried testing this PR and I immediately got this error

couldn't unmarshal config file: "strict mode: fields in the document are missing in the target struct"
panic: couldn't unmarshal config file: "strict mode: fields in the document are missing in the target struct"

goroutine 1 [running]:
github.com/joshmedeski/sesh/v2/seshcli.App({_, _})
        /Users/joshmedeski/c/sesh/review/seshcli/seshcli.go:57 +0x180c
main.main()
        /Users/joshmedeski/c/sesh/review/main.go:23 +0x154

A couple of thoughts:

1. Error visibility: The current error message doesn't indicate which specific field(s) are causing the issue. Could we enhance the error to show exactly what's missing?

2. User experience: Having all sesh commands crash due to config validation feels harsh for the user experience. What would you think about adding a dedicated sesh config validate command instead? This would let users explicitly check their config when needed, while allowing other commands to continue working.

3. Default behavior: I'm curious about the reasoning behind making strict mode the default. Could you share your thoughts on this design choice?

Looking forward to your perspective on these points!

[randomdude16671]

Thank you for the review. I didn't get this error on my config because its really basic I presume. I like preferred strict mode by default as I feel aligns more with the TOML file type's nature of being only used of configuration purposes. And I think a sesh config validate command would be amazing!, but I'm not very experienced with go style flag parsing with "flag" (I'm working on my own thing too with flags and find it quite difficult getting used to as a beginner to making command line applications with such features), And for the error message visibility I'm pretty sure its possible by using go-toml's DecodeError functions. Also I presume that the error finds a key which isn't in the struct that I used to unmarshall it (I think i didn't all the proper structs), Sorry for the inconvenience. Can this pull request be drafted until I find a good fix for the errors?

[joshmedeski]

TOML file type's nature of being only used of configuration purposes

I understand what you mean here but since this is a personal configuration for someone's workflow I would prefer to avoid defaulting to strict mode. This will potentially cause sesh to crash for a lot of users and would force them to have to fix their config before they can start using sesh again.

So, I'd rather this be an option that a user can opt-into, rather than changing the default behavior:

strict_mode = true

I recommend keeping the current function as-is and adding a new function that can get the config in a strict way.

If you have trouble getting the CLI command working don't worry about it and we can merge the core idea here and I can iterate on it.

[randomdude16671]

Thanks, I'll be working on it!

[randomdude16671]

Update: right now, it works pretty well and I used your idea and it works very well, This new field:

[evaluation]
strict = true 

enables strict mode evaluation, here in evaluation, there can be more options but I can't think of anything that can be put there.
I'm thinking about a way to optimize it right now before opening this PR again. Will continue working!

[joshmedeski]

Great, please re-request a review when you're all done.

There's no need to be organized with your commit messages. I will do a squash and merge on the PR.

[randomdude16671]

Everything's looking good from my side! Thanks for reviewing my changes patiently and I apologize for any previous errors I might have made. Marking them as ready-for-review now!

[joshmedeski]

Great start here, just a few suggestions.

[joshmedeski]

How's progress going on this? Let me know when it's ready for another review.

[randomdude16671]

Yep! This is actually complete in my opinion. Sorry for the late response, was held up in some work. But just one heads up: there's a single fmt.Printf that I added for the human readable error part which I couldn't get working with log.Fatalf or slog to print to stderr or stdout.