cucumber / gherkin-utils

API for working with Gherkin documents
MIT License
9 stars 4 forks source link

Walk/pretty print comments #16

Closed xeger closed 1 year ago

xeger commented 1 year ago

NB: This is a port of cucumber/common#1738 authored by @aslakhellesoy. I've patched the changes over from the monorepo to encourage conversation, and also fixed outstanding bugs, but I am not intimately familiar with gherkin-utils. Please refer to the comments on the original PR for valuable discussion.

Summary

Details

Keep a stack of comments and pop/walk comments until the current AST node.

How Has This Been Tested?

I've just added a couple of unit tests - we need more thorough tests for this:

Screenshots (if appropriate):

Types of changes

Checklist:

xeger commented 1 year ago

@aslakhellesoy I followed your advice and fixed all of the bugs people were complaining about. There are some odd cases. For example, a comment above a """ is preserved whereas comments above a table (but below its parent) are stripped - and I'm not sure if there is a way to preserve them (nor do I care).

A common practice in my org is block-commenting an entire section of the file while running tests interactively. I added a pretty case to verify that works well and found that it works a bit oddly, but correctly, given pretty's normalization of blank lines.

Do I need to look into porting all of this to Java, as well, or is it sufficient to remain selfishly focused on fixing the VS Code extension?

mpkorstanje commented 1 year ago

Clicked the wrong button. Approved CI to run and turned PR back into Draft.

mpkorstanje commented 1 year ago

We are a bit short handed lately, I'm not sure when someone will be able to review this. I have flagged it for extra attention though.

xeger commented 1 year ago

Thanks for the update @mpkorstanje; I understand being short handed.

There is a blocking issue: newline expansion demonstrated by failing pretty test. No hurry until I get that fixed and add a few more corner cases. I'll ping someone if I think it's good enough to ship.

Any input/help before that time is welcome, of course! πŸ˜„ But this one's important to me so I'll do what I can to drag it over the line.

xeger commented 1 year ago

I realized that the pretty round-tripping behavior that I thought was buggy, was actually correct. This PR is ready to go IMHO. Happy to add more test cases if anyone has recommendations.

xeger commented 1 year ago

Sorry to pester @mpkorstanje, but it's been a few months and my organization, plus a few others, have been happily using these pretty improvements via a forked build of the Cucumber VS Code plugin. I'd say this counts as reasonable QA on Aslak's comment-preserving fixes. πŸ˜„

I'm sure there are corner cases to be handled, and nobody in my sphere is blocking (we're all using the forked build) - but in the interest of enhancing the cucumber org's presence in the VS Code ecosystem, y'all might want to consider merging this stuff and kicking out new releases of the "official" plugin and its dependent libraries.

mpkorstanje commented 1 year ago

Currently everyone working on the project is a volunteer. It would be worth reading: https://mattwynne.net/new-beginning for a little bit more context. Unfortunately the public response from Smart Bear so far has been a deafening silence.

From a more practical point of view. We currently don't have anyone actively maintaining the VSCode plugin in the Cucumber org. Since you do have that motivation, would you be interested in turning the fork into the "official" version?

It would probably be easiest if you pop into the #commiters channel on Slack and/or join the weekly conference call to organize that. We are running a pretty informal process.

xeger commented 1 year ago

Thank you for the context. I'm leaning toward yes. I'll do some reading over the weekend and if I can commit, I will come say hi. πŸ™‡

mpkorstanje commented 1 year ago

Thank you for the context. I'm leaning toward yes. I'll do some reading over the weekend and if I can commit, I will come say hi. πŸ™‡

Cheers. Appreciated.

Commitment wise it would be comparable to running the current fork. The release process is automated. Everything else should be more or less comparable.

mattwynne commented 1 year ago

Thank you for the context. I'm leaning toward yes. I'll do some reading over the weekend and if I can commit, I will come say hi. πŸ™‡

Cheers. Appreciated.

Commitment wise it would be comparable to running the current fork. The release process is automated. Everything else should be more or less comparable.

πŸ‘ let us know if you have any questions @xeger!

xeger commented 1 year ago

I've copied gherkin/testdata/**/*.feature from an old commit of the monorepo and fixed the relative paths so that prettyTest iterates over all of the good examples. Happily, everything is still green.

This PR is ready to go.

aslakhellesoy commented 1 year ago

Hi @xeger,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

On behalf of the Cucumber core team, Aslak HellesΓΈy Creator of Cucumber