-
Notifications
You must be signed in to change notification settings - Fork 7
Add languages parameter #125
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
Conversation
| err := h.Validator.Struct(request) | ||
| if err != nil { | ||
|
|
||
| if _, ok := err.(*validator.InvalidValidationError); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L28-L41 I think we can reuse this logic across handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could! But I don't fully understand how. Ideally, we'd make a middleware out of it but with the current contract we'll parse the payload twice (unless you know the way to pass the struct along with the request)
| languages, err = pm.detectLanguages(project) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Failed to detect project languages") | ||
| removeProjectDir(projectPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but I think we need to nit error handling and have a defer rollback. This block is repeated too many times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do we really need to write to a channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run project creation in a goruotine because it takes a while. That's why we used channels. But I'm open to any suggestions here. This one was my first goroutine, so it's far from ideal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result := <-h.Manager.CreateProject(r.Context(), request) in handler is equivalent to method invocation as far as I can tell. But let's take it outside this PR.
d15634b to
9541177
Compare
This PR adds the
languagesparameter in the CreateProjectRequest to give users more control over which lsp servers to run in their projects. The provided languages are used instead of language detection.This PR also introduces request validation powered by https://github.com/go-playground/validator
For example, the valid values for the
languagesfield arePython,JavaScript,Go, andTypeScriptwhich are the only lsp servers we support right now. I wished we could use the variables in struct tags but for now we have to enter values by hand.I also added some filters recommended for language detection, not that I see any immediate difference, but I think those are nice to have.