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

Add Euphoria language #5241

Merged
merged 21 commits into from
Feb 21, 2022
Merged

Add Euphoria language #5241

merged 21 commits into from
Feb 21, 2022

Conversation

ghaberek
Copy link
Contributor

@ghaberek ghaberek commented Mar 1, 2021

Description

Add support for the Euphoria language (website, github). We have been gradually migrating development of Euphoria onto GitHub but the frequent mis-classification by linguist makes it look like we're using a handful of other languages (namely E, Eiffel, and Elixir) due to extension collisions. Euphoria primarily uses .e for include files and .ex for programs. These changes should correct that. I've done some diligence in correcting new false-positives by adding a sample for Elixir and several patterns to the heuristics, although admittedly I am not nearly as familiar with those other languages. I did test linguist against several larger Eiffel and Elixir repositories.

Checklist:

@ghaberek ghaberek requested a review from a team as a code owner March 1, 2021 07:21
@ghaberek ghaberek closed this Mar 1, 2021
@ghaberek ghaberek reopened this Mar 1, 2021
@ghaberek
Copy link
Contributor Author

ghaberek commented Mar 1, 2021

Apparently I didn't properly commit the git submodule for vscode-euphoria, probably because I synced my fork at some point and left that out. I've committed the submodule now, and that corrected the failing Ruby tests.

I'm still not sure how to correct the cross-validation test. I'm not even sure what's wrong. Running bundle exec script/cross-validation --test isn't covered in the contributing doc but bundle exec rake test completes successfully on my system.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

This is looking great. Thanks. 🙇 One nit about the extensions inline and an explanation for the failing test:

The classifier test uses just the samples in Linguist to test the classifier in isolation. It is failing because the classifier is detecting the Elixir/function.ex file as Eurphoria (it's buried here):

Elixir/function.ex BAD (Euphoria)

This will be happening because the content of that sample matches more attributes of Euphoria based on the samples provided than Elixir, which isn't really surprising because Elixir has only one other file and it's teeny weeny and really not representative of the language.

The solution is to add another Elixir .ex file that is a good representation of the language.

This wasn't an issue before now because Elixir was was added to Linguist many many years before we started curating samples so one wasn't added and it was the only language using .ex until now 😁 .

And thanks the for the pointer that the docs are missing mention of running this test.

lib/linguist/languages.yml Outdated Show resolved Hide resolved
@ghaberek
Copy link
Contributor Author

ghaberek commented Mar 2, 2021

I added several more samples for Elixir and that seems to have pleased the cross-validation script. 👍

@ghaberek
Copy link
Contributor Author

ghaberek commented Mar 5, 2021

Oops, didn't realize I need to mark that resolved. Also pulled in the latest change from upstream. 👍

@lildude lildude requested review from Alhadis and lildude March 8, 2021 10:00
@ghaberek
Copy link
Contributor Author

Just to clarify, is there anything else needed from my end to get this accepted?

@lildude
Copy link
Member

lildude commented Mar 15, 2021

Not at the moment thanks. @Alhadis may have some suggestions around the regexes when he gets a chance to have a look, but nothing else is needed at the mo.

@lildude
Copy link
Member

lildude commented May 14, 2021

@ghaberek can you please address the conflicts 🙇

@Alhadis when you've got a mo, do you mind taking a look at these regexes 🙇

@lildude
Copy link
Member

lildude commented Jul 19, 2021

Nudge

@Alhadis
Copy link
Collaborator

Alhadis commented Jul 19, 2021

@Alhadis when you've got a mo, do you mind taking a look at these regexes 🙇

Crap. 😞 Sorry. I'll have a look now.

lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
lib/linguist/heuristics.yml Show resolved Hide resolved
@lildude
Copy link
Member

lildude commented Feb 8, 2022

Nudge @ghaberek 😉

ghaberek and others added 2 commits February 8, 2022 20:02
@ghaberek ghaberek requested a review from Alhadis February 9, 2022 03:10
lib/linguist/heuristics.yml Outdated Show resolved Hide resolved
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Most of these samples are massive because it appears (from my naïve perspective) that they contain a lot of comments and a fair amount of duplication.

Please swap these for smaller samples with fewer/smaller comments.

@ghaberek
Copy link
Contributor Author

Please swap these for smaller samples with fewer/smaller comments.

All set. Unless those are still too big? A lot other files in the samples/ directory are well over 1k lines.

lildude
lildude previously approved these changes Feb 16, 2022
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Much better samples, thanks. 🙇

@lildude
Copy link
Member

lildude commented Feb 16, 2022

Much better samples, thanks. 🙇

Oh, maybe not.

@lildude lildude dismissed their stale review February 16, 2022 11:24

Tests are failing with these samples.

@ghaberek
Copy link
Contributor Author

Tests are failing with these samples.

TestHeuristics#test_ex_by_heuristics [/home/runner/work/linguist/linguist/test/test_heuristics.rb:23]:
no fixtures for Euphoria *.ex

So I just need to provide some .ex samples in the samples/Euphoria folder?

@lildude
Copy link
Member

lildude commented Feb 17, 2022

So I just need to provide some .ex samples in the samples/Euphoria folder?

I don't think so.

Update: Whoops, yes... for that error.

For the current failures:

I think you need to improve your heuristic as it's not detecting one of the files you're supplying correctly so leaving it to chance with the classifier.

If we're going to add samples, it would be for Eiffel as there are only 3 very old samples, but I think improving the heuristic would be a better approach for now.

@ghaberek
Copy link
Contributor Author

I don't think so.

Update: Whoops, yes... for that error.

I figured that so added more Euphoria samples with .ex yesterday.

I think you need to improve your heuristic as it's not detecting one of the files you're supplying correctly so leaving it to chance with the classifier.

The original Eiffel heuristic I added was only looking for left-anchored keywords, but since any Eiffel keyword is also a valid identifier in Euphoria, things like from = 2 in Euphoria would match Eiffel's keyword pattern as ^\s*(?:from)\s. I've added two new patterns to the Eiffel heuristic to match variable and function declaration syntax.

If we're going to add samples, it would be for Eiffel as there are only 3 very old samples, but I think improving the heuristic would be a better approach for now.

I've added a few more Eiffel samples.

@lildude lildude merged commit 0718213 into github-linguist:master Feb 21, 2022
@ghaberek
Copy link
Contributor Author

Thanks for accepting this! It's going to be a big help to our community. It looks like syntax highlighting is working now on Euphoria files in my projects, but I'm not seeing the "Languages" statistic update on anything yet. What does it take to make that happen?

@Nixinova
Copy link
Contributor

Looks fine to me
image
image

@ghaberek
Copy link
Contributor Author

Strange. I went back to the OpenEuphoria page and it was correct there as you showed but not on the Euphoria project itself.

I just took this screen shot. But then I did a Shift+F5 and it updated to what you show. False alarm I guess. Thanks again!

image

@lildude
Copy link
Member

lildude commented Feb 23, 2022

You need to push a change to the repo that will cause a re-analysis. Changes to .gitattributes, the only file changed in that repo since that new release was deployed, does not cause a re-analysis.

@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants