Skip to content

Add GDScript lexer #1457

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

Merged
merged 10 commits into from
Jun 1, 2020
Merged

Add GDScript lexer #1457

merged 10 commits into from
Jun 1, 2020

Conversation

pfertyk
Copy link
Contributor

@pfertyk pfertyk commented May 19, 2020

Note: this is WIP, not submitted for review yet. I need to add more tests and probably update the lexer, but for now I just want to run all pygments check.

The original PR is #1298 , I've reused all the code from it and will add required fixes.

@Anteru
Copy link
Collaborator

Anteru commented May 21, 2020

Can you please add an example file as well? Can be small, but it checks that the discovery works properly and makes it easy to test the lexer if needed.

@pfertyk
Copy link
Contributor Author

pfertyk commented May 21, 2020

Example file added, named gdscript_example.gd, because there was already an example.gd file (for GAP declaration). If I can remove that old file (I'm not sure if there is a lexer for it), let me know ;)

@pfertyk
Copy link
Contributor Author

pfertyk commented May 21, 2020

OK, my bad, there is a lexer, but in the file algebra.py, so I didn't find it initially.

@pfertyk
Copy link
Contributor Author

pfertyk commented May 21, 2020

Could you please confirm that the copyright notice in both the lexer and the test file is correct? I've basically copied the lexer from Godot Docs repository, keeping the original notice, but I've added one to the test file on my own, using the format from other test files.

I'm also not sure if I should include a note somewhere that the lexer was copied from Godot Docs repo, so that we know where to look for it later, if it needs to be updated. What is your opinion?

Other than that, I'll remove the WIP status from this PR and I think it's ready for a review. I've noticed that the lexer could do a bit more than it's doing right now (e.g. returning Token.Name.Function for functions or maybe returning Token.Keyword or Token.Name.Builtin.Type for void), but I think that can be done as an update, as the current version is "approved by Godot team" (they are using it in the documentation). Another thing I'd love to add is a style for Godot Editor, but that can be done in a separate PR I think (I'll create it soon).

@pfertyk pfertyk changed the title WIP Add GDScript lexer Add GDScript lexer May 21, 2020
@Anteru
Copy link
Collaborator

Anteru commented May 21, 2020

If .gd is used for another language already, you should implement analyse_text to disambiguate which lexer to use. Otherwise, you might get the wrong one when calling it on a .gd file. Regarding the copyright -- that'll depend if the godot folks are ok with it, but I believe they use a compatible license so it should be fine?

@Anteru
Copy link
Collaborator

Anteru commented May 22, 2020

Ok, so the license must be 2-clause BSD. See Contributing.md for details. @akien-mga: Is it possible for godot to relicense this under the 2-clause BSD, so I can merge this into Pygments and redistribute? Our contribution guidelines require 2-clause BSD (and ideally you could adapt our copyright notice as well, but IANAL.)

@pfertyk
Copy link
Contributor Author

pfertyk commented May 23, 2020

I've added analyze_text method to both lexers (GAP and GDScript) and added tests for that.

GDScript lexer that I've copied is licensed under MIT, but the whole Godot Docs repository uses Creative Commons Attribution 3.0 Unported. I think MIT is compatible with BSD, but I'm also not a lawyer ;)

@akien-mga
Copy link

I confirm that the GDScript lexer is under the MIT license, but if it would be necessary to have it under BSD to integrate seamlessly in pygments, a relicensing would definitely be an option.

The original author @djrm hasn't been active on GitHub in a while, I'll see if I can reach out to him by email to agree to the relicensing.

@djrm
Copy link
Contributor

djrm commented May 23, 2020

Sure, go ahead 👍, we just had to wait 3 years or something like that for this to be considered for merging...

@Anteru
Copy link
Collaborator

Anteru commented May 24, 2020

Sorry about the long wait, but happy to hear that we can make this work. It won't take 3 years until you see the next release :)

@Anteru
Copy link
Collaborator

Anteru commented May 26, 2020

@pfertyk Can you please update the PR with the changed license?

@pfertyk
Copy link
Contributor Author

pfertyk commented May 26, 2020

@Anteru sure, sorry for the delay. I've changed it to BSD, like in other lexers.

@Anteru Anteru self-assigned this Jun 1, 2020
@Anteru Anteru added the changelog-update Items which need to get mentioned in the changelog label Jun 1, 2020
@Anteru Anteru added this to the 2.7 milestone Jun 1, 2020
@Anteru Anteru merged commit e5dc231 into pygments:master Jun 1, 2020
@Anteru
Copy link
Collaborator

Anteru commented Jun 1, 2020

Merged, thanks for the contribution. Hope we'll get this out before Godot 4.0 :)

@akien-mga
Copy link

Great, thanks :)

@vnen FYI - once the syntax for GDScript "2.0" is finalized for Godot 4.0 in a few months, we should make sure to update the lexer here (ideally keeping support for both "1.0" and "2.0" syntax as they will likely coexist for a while - and differences won't be big enough to warrant treating them as separate langs IMO).

@Anteru Anteru removed the changelog-update Items which need to get mentioned in the changelog label Jun 1, 2020
@pfertyk
Copy link
Contributor Author

pfertyk commented Jun 1, 2020

@Anteru Thanks for merging, I'm glad I could help ;)

@akien-mga I'll keep my eyes open for GDScript 2.0. If the differences are small, I think it could be added the same way as Python 2 and Python 3 lexers.

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