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

#71: support either std and exp slog #72

Closed

Conversation

uroborosq
Copy link

Attempt to achieve support of either log/slog and golang.org/x/exp/slog.

In some cases golang.org/x/exp/slog is still using, although is looks more like legacy.


Store now list of possibe imports in dedicated slice.

Maps with funcs and attrs are generated once from templates using both import names
Type names comparing with both import names too.


  • implement tests. looks attractive just to copy whole tests dir with alternative

@ccoVeille
Copy link

I'm not a maintainer, but I'm surprised by the complexity of the changes.

The readability and maintainability is highly affected

Did you consider using something like this

func normalizeExp(name string) (string, bool) {
	normalizedNamed, found := strings.CutPrefix(name, "golang.org/x/slog")
	if found {
		normalizedNamed = "slog" + normalizedNamed
	}
	return normalizedNamed, found
}
        name, isExp := normalizeExp(fn.FullName())
        funcInfo, ok := slogFuncs[name]
        if !ok {
                return
        }

Then keep all the code unchange, and use isExp if needed

@uroborosq
Copy link
Author

Thanks for comment, I got your point, I see you offer to reduce code changes.

Basically, I wanted to have one single entrypoint for all possilble type names. Is there a situation when we don't need to use isExp?
In the dependent code I still have just to search once in a map, I don't need to know if there are some exp or std versions of name types. I find it more transparent, then I have all possible names in just one map.
From the first look you can see, that these names are templates, so there can be several final names. The map with final names is just below them.
You have some names in the map, which looks pretty completed, but then you cut somewhere prefixes before search - from my side it can be not so obvious, it requires to get into detail. You need to find map, than to find its usages and understand that you also need to check for exp import. So in this case I would wrap it all in the one function, and hide other details like map and this func from dependent code. But still I personally found situation when all names stored in one map simpler, although diff is larger.

Also I want to point, that checking prefix wouldn't be enough, because in some places full name of type is placed inside brackes (for pointers). So you need check contains, which slightly has more chance to get a false positive. Although I don't think it has any chance to happen.

To add new import name supported, I can just it to slice, not adding new if constructions.

Anyway, I admit that my approach might be overhead for such small project, so I will accept any suggestions from maintainers without objection.
Just want to have this feature in the main asap

@tmzane tmzane self-requested a review February 16, 2025 19:14
@uroborosq
Copy link
Author

close this, reasons are in the related issue - this changes are not worth to support in the future

@uroborosq uroborosq closed this Feb 17, 2025
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