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 .wx extension to Monkey language #5964

Closed
wants to merge 11 commits into from

Conversation

seyhajin
Copy link

@seyhajin seyhajin commented Jul 5, 2022

Added Wonkey extension to Monkey language.

Description

Wonkey is a up to date fork of Monkey2 programming language for the creation of cross-platform video games (very similar to BlitzBasic/BlitzMax but with oriented object programming).

preview:

struct Vec2<T>
    field x:T
    field y:T

    method new( x:T, y:T )
	self.x = x
	self.y = y
    end

    operator+:Vec2( v:Vec2 )
        return new Vec2( x + v.x, y + v.y )
    end

    operator to:string()
        return "Vec2(" + x + ", " + y + ")"
    end
end

alias Vec2<float> Vec2f

'main entry
function main()
   local v0:= new Vec2f( 10,20 )
   local v1:= new Vec2f( 30,40 )
   print v0 + v1
end

Wonkey : https://github.com/wonkey-coders/wonkey
Monkey2 : https://github.com/blitz-research/monkey2

Checklist:

Added Wonkey extension to Monkey language
@seyhajin seyhajin requested a review from a team as a code owner July 5, 2022 20:19
@Alhadis Alhadis changed the title Update languages.yml : added Wonkey extension to Monkey language. Add .wonkey extension to Monkey language Jul 6, 2022
@Alhadis Alhadis changed the title Add .wonkey extension to Monkey language Add .wx extension to Monkey language Jul 6, 2022
@Alhadis
Copy link
Collaborator

Alhadis commented Jul 6, 2022

First, please fill out the pull-request template. It's there for a reason.

Second, a little background on what "Wonkey" is would be nice. I notice that you're adding .wx as an extension, not .wonkey, so adding a link or a short explanation explaining what "Wonkey" is would help to clear up any confusion.

@seyhajin
Copy link
Author

seyhajin commented Jul 6, 2022

OK, i have updated my pull request description with more informations.

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.

  • I have included a real-world usage sample for all extensions added in this PR:

No you haven't 😉 Please add the sample to the PR.

@lildude
Copy link
Member

lildude commented Jul 13, 2022

I'd add the sample myself, however you've not granted maintainers write access to your branch so I can't. Accordingly, this PR will miss the release I'm making today.

@seyhajin
Copy link
Author

No you haven't 😉 Please add the sample to the PR.

@lildude , Ok i added a Vec2 sample

I'd add the sample myself, however you've not granted maintainers write access to your branch so I can't. Accordingly, this PR will miss the release I'm making today.

It seems not possible to "Allow edits from maintainers" from organization account pull request, see https://github.com/orgs/community/discussions/5634.

@seyhajin seyhajin requested a review from lildude August 10, 2022 07:39
@lildude
Copy link
Member

lildude commented Aug 10, 2022

@lildude , Ok i added a Vec2 sample

Where did you get this from? Please update the OP to link directly to the file.

@seyhajin
Copy link
Author

seyhajin commented Aug 10, 2022

@lildude , Ok i added a Vec2 sample

Where did you get this from? Please update the OP to link directly to the file.

Link updated and i added this file in PR.

@lildude
Copy link
Member

lildude commented Aug 10, 2022

Errm, we need a link to the original source, not your fork of Linguist 😉

@seyhajin
Copy link
Author

Errm, we need a link to the original source, not your fork of Linguist 😉

Cause i have added this file in this pull request but ok i have changed link from original vec2 source now. :)

@lildude
Copy link
Member

lildude commented Aug 10, 2022

Cause i have added this file in this pull request but ok i have changed link from original vec2 source now. :)

🤔 those two files don't match. If you've written the sample explicitly for this PR, then that's fine. You can state that instead of linking to a source.

@seyhajin
Copy link
Author

🤔 those two files don't match. If you've written the sample explicitly for this PR, then that's fine. You can state that instead of linking to a source.

I explicitly added a sample file in my linguist fork in the "samples/Monkey/vec2.wx" folder and integrated it into this PR.
What link do you want me to put in this PR?
I updated the link, tell me if it's good

Copy link
Author

@seyhajin seyhajin left a comment

Choose a reason for hiding this comment

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

Ok for me

@seyhajin
Copy link
Author

seyhajin commented Aug 25, 2022

@lildude Do you expect other actions from me before to merge this PR?

@lildude
Copy link
Member

lildude commented Sep 2, 2022

I've finally looked into the usage and there are a couple of issues:

Your search string suggests usage is nowhere near popular enough with only 904 file, most of which are owned by one user. Removing that user gives only 162 files indexed which is a long way from the documented and even temporarily relaxed requirements. I've tried several other combinations of the search for Wonkey specific keywords and I can't find a result that satisfies the requirements. If you can generate a representative search, please do so.

More importantly... inverting your search to exclude the word "import" (and exclude the wonkey-coders user) reveals this is going to misclassify a lot of other users of the .wx extension that are definitely not Wonkey.

In order to prevent this, you're going to need to identify at least one of those languages and add support to this PR along with two representative samples of both languages to ensure the classifier has a chance of getting things right. If the two languages are similar, you will need to implement a heuristic too.

@seyhajin
Copy link
Author

seyhajin commented Sep 3, 2022

I've finally looked into the usage and there are a couple of issues:

Your search string suggests usage is nowhere near popular enough with only 904 file, most of which are owned by one user. Removing that user gives only 162 files indexed which is a long way from the documented and even temporarily relaxed requirements. I've tried several other combinations of the search for Wonkey specific keywords and I can't find a result that satisfies the requirements. If you can generate a representative search, please do so.

More importantly... inverting your search to exclude the word "import" (and exclude the wonkey-coders user) reveals this is going to misclassify a lot of other users of the .wx extension that are definitely not Wonkey.

In order to prevent this, you're going to need to identify at least one of those languages and add support to this PR along with two representative samples of both languages to ensure the classifier has a chance of getting things right. If the two languages are similar, you will need to implement a heuristic too.

Ok i've changed search filters by adding original "Monkey, Monkey2" extensions and removing 'import' keyword because is a same language, only extension name has been simplified for Wonkey.
I find it very complicated just for adding an extension at existing language :-/

@lildude
Copy link
Member

lildude commented Sep 5, 2022

Ok i've changed search filters by adding original "Monkey, Monkey2" extensions and removing 'import' keyword because is a same language, only extension name has been simplified for Wonkey.
I find it very complicated just for adding an extension at existing language :-/

It's not that complicated... it's just that we need to ensure each extension meets the popularity requirements before inclusion so adding the Monkey extensions to your query doesn't help illustrate the popularity of the .wx extension 😁. We also need to ensure that other languages that share the extension are not incorrectly classified.

@seyhajin
Copy link
Author

seyhajin commented Sep 6, 2022

It's not that complicated... it's just that we need to ensure each extension meets the popularity requirements before inclusion so adding the Monkey extensions to your query doesn't help illustrate the popularity of the .wx extension 😁. We also need to ensure that other languages that share the extension are not incorrectly classified.

@lildude I've reduced extension filter to only '.wx' and of course, Wonkey is not the only one to use this extension, should we add more code examples to facilitate the classification or it would not be useful?

@lildude
Copy link
Member

lildude commented Oct 20, 2022

@lildude I've reduced extension filter to only '.wx' and of course, Wonkey is not the only one to use this extension, should we add more code examples to facilitate the classification or it would not be useful?

Yes. I think we're also going to need to identify the other predominant user of this extension and add support for it with at least two samples to this PR too.

@seyhajin
Copy link
Author

@lildude I've reduced extension filter to only '.wx' and of course, Wonkey is not the only one to use this extension, should we add more code examples to facilitate the classification or it would not be useful?

Yes. I think we're also going to need to identify the other predominant user of this extension and add support for it with at least two samples to this PR too.

I added more Wonkey samples

@lildude
Copy link
Member

lildude commented Nov 14, 2022

I added more Wonkey samples

Woah! Some of those are bigger than we need. If the diff view collapses the file, it's too big. Please remove these large samples.

You're still going to need to identify the language of at least one other user of the .wx extension and add it as part of this PR.

@seyhajin
Copy link
Author

Woah! Some of those are bigger than we need. If the diff view collapses the file, it's too big. Please remove these large samples.

OK i've removed large sample files.

You're still going to need to identify the language of at least one other user of the .wx extension and add it as part of this PR.

I've updated the PR.

@seyhajin seyhajin requested a review from a team as a code owner January 21, 2024 10:11
@lildude
Copy link
Member

lildude commented Jan 21, 2024

You're still going to need to identify the language of at least one other user of the .wx extension and add it as part of this PR.

👆

@lildude
Copy link
Member

lildude commented Jul 10, 2024

Closing as the requested changes have not been made in well over a year since being requested. Please open a new PR when you have the time to make the requested changes.

@lildude lildude closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants