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

Switch to peggy, and add support for @ operator #5

Closed
wants to merge 2 commits into from
Closed

Switch to peggy, and add support for @ operator #5

wants to merge 2 commits into from

Conversation

hildjj
Copy link

@hildjj hildjj commented Apr 23, 2021

Hi there! We've moved development on pegjs to a community fork called "Peggy". I use your extension all the time, and wonder if you might be willing to move to peggy to support some of the new features that we just released.

This is a minimal patch that does s/pegjs/peggy/g and adds support for our new @ operator.

@SrTobi
Copy link
Owner

SrTobi commented Apr 23, 2021

Hi @hildjj. First of all let me say that it's great to see some new development going on for peg.js!

I looked at your PR and was close to merge it, also because I want to support any new development.
But to be honest I think it is the wrong move to convert this repo into supporting the new peggy language and suggest to fork this repo and create a new extension in the vscode store instead.
Here are a few reasons:

  • from what I see you have planned quite a lot of things and I guess it would be a hurdle to always go through me for an update of the vscode extension
  • Especially if you want to add a lot of new features (like formatting and such things), I think you will be more flexible when you have your own repo.
  • At the moment the peggy-definition-language might be a superset of peg.js, but that might change in the feature, in that case users of the old language will not be supported anymore and I wouldn't want that.
  • New users of peggy will search for "Peggy" in the marketplace and I think it makes more sense to have a plugin specifically named for them (nothing shows up there yet).
  • Just hypothetically, your pr (especially with subsequent updates of the lib) would be a perfect supply chain attack. I'm not saying that is the case 😃, but I'm also not willing to scan through all of the commits in the peggy repo. Of course, it is not the case that I have checked any of the other libraries, but your project is only 2 weeks old and I can imagine other users not wanting me to put in some new library that I haven't checked that will run on their machines. I know that everytihng we do here is basically a security risk, but maybe let's not perpetuate it.

So, I suggest you just fork this repo (maybe incorporate some of the changes from this copy-cat extension 😹) and upload your own extension called peggy-language. I would, of course, be flattered if you mention me.
I want to still be supportive of your work, so let's say if your development continues for 2 more month, I will add something to the readme that suggests to the user to switch to your extension AND your new version of the language/parser.
I think this would be the perfect solution for all parties involved. What do you think?

@SrTobi SrTobi self-assigned this Apr 23, 2021
@hildjj
Copy link
Author

hildjj commented Apr 23, 2021

That makes sense to me. I'll ping you when we get something up and running, then again in a few months for you to measure our progress. In the meantime, we would welcome any contributions to peggy or our vscode extension fork by you or your users.

Thanks for your quick response!

@SrTobi
Copy link
Owner

SrTobi commented Apr 23, 2021

Ok, cool. Thanks again for taking up the development of peg.js again!

@SrTobi SrTobi closed this Apr 23, 2021
@hildjj
Copy link
Author

hildjj commented Apr 24, 2021

See https://marketplace.visualstudio.com/items?itemName=PeggyJS.peggy-language

We would love feedback from anyone that tries it.

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.

2 participants