PHP language support about gritql HOT 43 OPEN

@abhishek818 @urbit-pilled @iamnamananand996 I will assign this bounty to someone once they land the first main PR (ex. adding the grammar + parse test + initial metavariables).

from gritql.

@urbit-pilled asked about the prefix to use since PHP uses $ for its own variables.

For now, just override the metavariable to something like %. This is simplest.

Alternative, we can use $[name] for Grit metavariables in PHP, since $[name] is not a valid PHP variable.

from gritql.

@ayewo battle royale style is a lot riskier but I think it can work okay as long as the submissions are easy to measure and are all compared against a well defined benchmark.

from gritql.

Thanks @urbit-pilled for landing the main PR here. I'll award $1k from the bounty for that. There's still a bit more work to be done to consider this complete:

• PHP templates (ie. in HTML) should be supported as an alternative flavor (like we have in JS) with language php(html)
• We definitely need more test coverage, I'd like at least 5-6 more cases.

from gritql.

I'll award $1k from the bounty for that.

@morgante Algora has a command for making partial awards on GitHub. Simply add a comment on this issue with:
/tip $1000 @urbit-pilled

Apologies if you already know this.

from gritql.

Tests cases that should work:

• echo $_

from gritql.

🎉🎈 @urbit-pilled has been awarded $1,000! 🎈🎊

• Share on socials
• Give feedback

from gritql.

🎉🎈 @urbit-pilled has been awarded $250! 🎈🎊

• Share on socials
• Give feedback

from gritql.

Hi @urbit-pilled, before we consider a language "complete" we usually like to try it on a few repos and run some sample migrations. For this to be complete, I think I'd like to see:

• Run this on some larger open source codebases and make sure you don't see tons of parse errors
• Add 1-2 sample rules to the PHP stdlib: https://github.com/getgrit/stdlib/tree/main/.grit/patterns

from gritql.

/bounty $1500

from gritql.

/attempt #48

from gritql.

@iamnamananand996 Do you want to attempt this bounty?

from gritql.

@morgante can I get assigned?

from gritql.

/attempt #48
I've made a few tools using tree-sittter (https://github.com/urbit-pilled/tree-sitter-hoon and https://github.com/urbit-pilled/hoon-ts-editors) so I'd like to work on this

from gritql.

@iamnamananand996 Do you want to attempt this bounty?

hi @morgante, yes I am on it, look like a race now, 😃, let see who wins.

from gritql.

💡 @urbit-pilled submitted a pull request that claims the bounty. You can visit your bounty board to reward.

from gritql.

@morgante

The tree-sitter-php project adopted the same split as tree-sitter-typescript two months ago¹ where they have two dialects: php_only and php.

• php_only: which allows top level statements, start/end tags are optional;
• php: which parses both PHP and HTML (i.e. normal PHP parser since PHP started out as a templating language).

1. Which of these two grammars should we pick for the initial support? I imagine this will be php_only?
2. How do we approach naming of the two dialects within the Grit project? Naming is hard and agree that in an ideal world² it would have been better to use the names:
   • php_only => php
   • php => php_html

from gritql.

Hi @ayewo, thanks for flagging this.

I think we probably want to use php (with HTML) as the default, since that matches the largest number of PHP codebases in the wild.

For toggling between them, we should use the flavor mechanism we added for JS/TS here. This would support language php(html) or language php(only) with the default as php(only).

from gritql.

Finally got metavariables to work in my PR, this commit contains the biggest changes I had to make: :197575d

from gritql.

/assign @urbit-pilled

from gritql.

@morgante I just submitted a pull request that solves this issue why are you assigning this to someone else?

from gritql.

@digital-phoenix I'm sorry you went into the effort of submitting a pull request, but we can only award the bounty to one person. @urbit-pilled's PR is earlier and closer to completion.

from gritql.

@morgante @urbit-pilled's test cases are not even generating abstract syntax trees yet it is just doing regex comparisons. In order for php to see text as code it has to be contained within php tags. My test cases work not just as regex comparisons but as actual code.

from gritql.

I don't think <?php is required when using the php-only grammar.

from gritql.

@morgante I tested it before when I was adding php. Unless you use <?php the test will just be interpreted as text. Just using regex leads to issues such as not being able to match a function body when it is split across multiple lines.

from gritql.

Language support is usually not completed in a single PR. I plan to merge @urbit-pilled's PR and award part of the bounty, but will award additional bounties for PRs adding more test cases and fixing associated bugs.

from gritql.

@morgante @urbit-pilled's grammar file does not seem to even compile correctly. It seems to contain conflicts that have not been included in the conflict table I'm not convinced they have ever even used their own grammar file. Whereas my code includes support for a broad range of language features.

from gritql.

If the grammar file wasn't used I don't see how the test cases could pass.

This is an open source project, I prefer to have a collaborative community where we all focus on producing good code not tearing down each other's work.

from gritql.

@morgante I understand the importance of collaboration but you're backing code that doesn't work properly over everyone else's efforts

from gritql.

@morgante I understand the importance of collaboration but you're backing code that doesn't work properly over everyone else's efforts

90% of the work is the same in your PRs. It would not be fair to discard the earlier PR, even if it is incomplete.

As I noted, you will have an opportunity to add (and fix) more test cases for your share.

For what it's worth, this is making me see the problems with the bounty model. We put this out as an experiment, but it's becoming a lot of trouble to manage.

from gritql.

For what it's worth, this is making me see the problems with the bounty model. We put this out as an experiment, but it's becoming a lot of trouble to manage.

I was trying to point this out this very scenario playing out in Discord. Glad you've also come to the same conclusion—it can cause some bad blood for contributors and logistical headaches for projects that is completely avoidable.

As I said, battle royale style, only works well if the bounty is a few hours of work. This is a multi-day PR.

from gritql.

@morgante if you are going to have a public bounty either you need to accept whoever provides the first work that solves the requirements or you need to assign the task to someone.

Most of the code will be the same because this project requires a lot of boiler plate to add a language. But the most important part the changes to the base grammar files are different.

from gritql.

@digital-phoenix As far as I know, I am not required to accept any pull request. If you prefer not to contribute to our repository, I understand, but I'm just trying to be fair to everyone. You knew there were already PRs open, that I was engaged with and reviewing, and chose to start a new PR anyways.

I didn't assign it to anyone up front because, frankly, many contributors aren't prepared to work on something like this and I wanted to see material demonstration of ability before putting it on a single person. I have learned my lesson and will avoid big bounties in the future.

from gritql.

@morgante if you base this off of whoever opens a PR first that just encourages people to open broken buggy PRs. Which will just make work for you.

from gritql.

@morgante @urbit-pilled's grammar file does not seem to even compile correctly. It seems to contain conflicts that have not been included in the conflict table I'm not convinced they have ever even used their own grammar file. Whereas my code includes support for a broad range of language features.

@morgante @urbit-pilled's test cases are not even generating abstract syntax trees yet it is just doing regex comparisons. In order for php to see text as code it has to be contained within php tags. My test cases work not just as regex comparisons but as actual code.

You may be testing an older version? Because the grammar file does compile correctly. And the test cases are not regex matches, and the php tags are optional if you set php_only as the default which was requested earlier in the comment thread.

I think you started too late @digital-phoenix , because your PR doesn't implement the prefix change (^ instead of $) or support php without tags yet. I already finished what you did in the first day of my PR (get php working without prefix), however you did add more tests.

Sumbitting a PR this late when someone else is nearly finished is bad, especially when you haven't fully fixed the issue or caught up to my progress yet.

I am new to bounties and this is the first time I've had a bad experience with another bounty hunter trying to do this.

from gritql.

/tip $1000 @urbit-pilled

from gritql.

👉 @morgante: Navigate to your dashboard to proceed

from gritql.

Thanks morgante, I'll work on the rest today.

from gritql.

hey @morgante you need to click on the link posted by algora-pbc to award the tip. it hasn't been awarded yet.

👉 @morgante: Navigate to your dashboard to proceed

from gritql.

Tests cases that should work:

• echo $_

just added a test case for this in crates/core/src/test.rs

`echo ^_;` => `print "modified;`

However instead of echo $_ we use echo ^_ because:

• the prefix in php is ^ instead of $
• there is a semicolon needed due to: #52 (comment)

from gritql.

/tip $250 @urbit-pilled

from gritql.

@algora-pbc what are the correct commands to use when paying out a bounty incrementally?
Because I haven't received any of it yet...

from gritql.

Thanks Morgante, I received the $1,250 tip. Anything else that needs to be done for the rest of this bounty @morgante?

from gritql.