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

ensureConfigLoaded masks errors #20

Closed
perbu opened this issue Nov 13, 2023 · 5 comments
Closed

ensureConfigLoaded masks errors #20

perbu opened this issue Nov 13, 2023 · 5 comments
Labels
feature New feature or request help wanted Extra attention is needed

Comments

@perbu
Copy link
Contributor

perbu commented Nov 13, 2023

The following code is used quite a bit. The problem is that it will mask all sort of errors behind "section not found". It probably not possible to change this without breaking the API, though. Not sure if you want a suggested fix.

func (t *tree) ensureConfigLoaded(config string) (*config, bool) {
	cfg, loaded := t.configs[config]
	if !loaded {
		if err := t.loadConfig(config); err != nil {
			return nil, false
		}
		cfg = t.configs[config]
	}
	return cfg, true
}

Cheers,

Per.

@dmke
Copy link
Member

dmke commented Nov 13, 2023

I agree, that bit isn't particularly elegant.

I'm not against breaking the API and build a v2, however I haven't got any time for this currently...

@dmke dmke added feature New feature or request help wanted Extra attention is needed labels Nov 13, 2023
@perbu
Copy link
Contributor Author

perbu commented Nov 13, 2023

No problem. I understand you're willing to take patches. I'd be happy to do all the work. We're starting to lean on this code a bit so we wanna give it some love in the months to come.

@perbu
Copy link
Contributor Author

perbu commented Nov 15, 2023

I got something that is starting to look ok. One question is whether or not we wanna keep the deprecated functions or they are to be removed from the v2 package?

If you wanna check out what I've done so far I'll raise a draft PR so you can have a look at the current diff: #21

@dmke
Copy link
Member

dmke commented Nov 15, 2023

I've created a v2 branch. If you want, you can create a PR against that branch.

@perbu
Copy link
Contributor Author

perbu commented Nov 21, 2023

This is now resolved. Thanks for taking the time to process the patch.

@perbu perbu closed this as completed Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants