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

Functions & Structures 🎉 #156

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

hapily04
Copy link

@hapily04 hapily04 commented Feb 5, 2024

No description provided.

@hapily04 hapily04 changed the title Functions Functions & Structures 🎉 Feb 5, 2024
Copy link
Author

@hapily04 hapily04 left a comment

Choose a reason for hiding this comment

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

I meant for the license to be only for my fork please ignore/decline this change

@WeeskyBDW
Copy link
Contributor

WeeskyBDW commented Feb 6, 2024

Hello
First at all, welcome and thanks to contribute to the project. Dont stress yourself with git stuff, if necessary a skript-parser maintainer would (help you to) fix theses. Also be carefull when changing forked project licence as if you didn't ask to projet author (and contributors) you can, in open-source project (mostly with biggest ones) have legal issues (intellectual property/copyright)

WIll see if i can be usefull about that PR but prob not

@Mwexim
Copy link
Owner

Mwexim commented Feb 11, 2024

I see a lot of interesting changes but also some things that do not follow the project's guidelines. Functions have never really been discussed.

Therefore, I will leave this pull request open for discussion and once I find some more time to work on the project again, I will happily talk about this pull request with you.

The concerns I currently have:

  • Code style, the java version upgrade
  • The function pattern is the same as in vanilla Skript, but nobody actually discussed if an overhaul would be preferred
  • The code changes some minor things that have nothing to do with the pull request
  • No tests were added and no description is provided.

@hapily04
Copy link
Author

hapily04 commented Feb 13, 2024

I see a lot of interesting changes but also some things that do not follow the project's guidelines. Functions have never really been discussed.

Therefore, I will leave this pull request open for discussion and once I find some more time to work on the project again, I will happily talk about this pull request with you.

The concerns I currently have:

  • Code style, the java version upgrade

To be quite frank the java version upgrade appears to be for the (x instanceof y variableName) which I use in my own code typically. I originally made all of these changes/additions/fixes for my own fork, but thought it'd be useful to share.

  • The function pattern is the same as in vanilla Skript, but nobody actually discussed if an overhaul would be preferred

Like I said before, a lot of these things are for my own fork which aims to be pretty similar to Skript so that it's easier for anyone wanting to switch. If there was a better proposal for function syntax I wouldn't be immediately opposed to still contribute and edit my contributions, but will keep my implementation for my own fork.

  • The code changes some minor things that have nothing to do with the pull request

I agree and this has a lot do with me not committing correctly in preparation for a pull request. I'm unsure how to fix any of this without going through the burden of creating a new fork and adding my changes one by one.

If you could help guide me in how to fix both of my pull requests (and potentially split some parts of them into different pull requests) that would be extremely helpful.

@Mwexim
Copy link
Owner

Mwexim commented Feb 14, 2024

Feel free to keep adding pull requests for features you're adding to your own fork, it keeps the project updated and discussion going. I will always try to discuss the changes and help you through the process. The only thing I'd like to stay is the Java version.

This particular pull request though just changes too many things at once, and would need some major discussion and changes for it to be merged. It's also a pretty close copy of Skript's system, and skript-parser always tries to optimize things and see if we could handle things differently before copying certain features. Did you have this mind-set when adding functions to your fork?

I currently lack the time to guide you through this as I would need to think for myself if Skript's way is indeed the right way to do things. That said, I really welcome your contribution and future contributions. If we ever add functions to skript-parser, this pull request will certainly add as a good reference on where to start!

Copy link
Contributor

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

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

Tests are failing

"string@s"
);
registration.newType(String.class, "string", "string@s")
.literalParser(s -> s)
Copy link
Contributor

@TheLimeGlass TheLimeGlass Mar 30, 2024

Choose a reason for hiding this comment

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

This does not take into consideration quotes. So this will treat anything as a simple literal. Not Ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say just no literal parser as variable string is supposed to be for string

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