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

Always map enry->bblfsh language names #251

Merged
merged 2 commits into from
Feb 14, 2019
Merged

Conversation

bzz
Copy link
Contributor

@bzz bzz commented Feb 13, 2019

This fixes https://github.com/bblfsh/client-go/issues/108

Before, we were mapping enry->bblfsh language name only in case it was detected by bblfshd itself (by calling to enry).

Now same change is applied on any user input, as the main use case for the clients is to detect language with Enry first and then call bblfshd with it, but language names are different (e.g C# vs csharp)

This is a workaround, until proper multiple language_aliaces are supported by every driver in it's manifest.

TEST PLAN:

  • $ bblfsh-cli -l 'C#'./some.cs

Signed-off-by: Alexander Bezzubov <bzz@apache.org>
daemon/language.go Outdated Show resolved Hide resolved
@@ -102,6 +102,8 @@ func (s *ServiceV2) selectPool(rctx context.Context, language, content, filename
return "", nil, err
}
language = lang
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this one need a normalize call too?

Copy link
Contributor Author

@bzz bzz Feb 13, 2019

Choose a reason for hiding this comment

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

This one will already have it normalized after s.detectLanguage that calls normalize underneath

Co-Authored-By: bzz <bzz@users.noreply.github.com>
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Unfortunately Github does not allow to comment lines not touched by the diff, but it may be necessary to list those aliases in SupportedLanguages as well. What do you think?

// TODO(bzz): remove this as soon as language aliases are supported in bblfsh
// driver manifest.
func normalize(languageName string) string {
lang := strings.ToLower(languageName)
lang = strings.Replace(lang, " ", "-", -1)
lang = strings.Replace(lang, "+", "p", -1)
lang = strings.Replace(lang, "#", "sharp", -1)
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting way of solving this :)

Do you think making it an explicit map makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interesting way of solving this :)

Well, it's the smallest possible change consistent with current behavior :)

As this is a workaround, I would prefer to keep this change as least invasive as possible.

Given that we do not have much tests for it and that the current approach already works (TM) for API calls \wo the language - I would postponed changing implementation until proper alias support.

@bzz
Copy link
Contributor Author

bzz commented Feb 13, 2019

it may be necessary to list those aliases in SupportedLanguages as well. What do you think?

I think this would be out of scope of this PR/Issue - it only supposed to allow for consumption of enry names, as requested in original issue.

And AFAIK such normalization is not even necessary for SupportedLanguages, as they already are a bblfsh names.

In my understanding, as discussed elsewhere, the consensus is to add a workaround so clients can call Parse() \w enry names, until we have a proper multiple aliases support.

@dennwc
Copy link
Member

dennwc commented Feb 13, 2019

OK, let's do it!

@bzz bzz merged commit 83166ea into bblfsh:master Feb 14, 2019
@bzz bzz deleted the always-remap-languages branch February 14, 2019 12:21
@bzz bzz self-assigned this Feb 22, 2019
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.

4 participants