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

Fix JSON Formatter #1580

Merged
merged 11 commits into from
Dec 7, 2021
Merged

Fix JSON Formatter #1580

merged 11 commits into from
Dec 7, 2021

Conversation

eraffel-MDSol
Copy link
Member

@eraffel-MDSol eraffel-MDSol commented Oct 30, 2021

Description

With the change to the JSON class, passing pretty as a parameter no longer does anything. pretty_generate should be called for the desired output.

End of the background is also broken if the user has Before hooks that come from libraries other than this one.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.

  • Tests have been added for any changes to behaviour of the code
  • New and existing tests are passing locally and on CI
  • bundle exec rubocop reports no offenses
  • RDoc comments have been updated
  • CHANGELOG.md has been updated

@eraffel-MDSol eraffel-MDSol marked this pull request as ready for review October 30, 2021 17:21
@eraffel-MDSol
Copy link
Member Author

Tests are passing locally

@eraffel-MDSol eraffel-MDSol changed the title Use Pretty JSON Formatter Fix JSON Formatter Nov 1, 2021
Copy link
Contributor

@aurelien-reeves aurelien-reeves left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

I have a few comments, but no big deal actually 💪

lib/cucumber/formatter/json.rb Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eraffel-MDSol
Copy link
Member Author

@aurelien-reeves Something else I noticed, the JSON formatter no longer has expanded docstrings for Scenario Outlines. Is there a reason to use the step_source from the lookup here rather than the multiline_arg from the test_step like it used to. https://github.com/cucumber/cucumber-ruby/blob/main/lib/cucumber/formatter/json.rb#L178

It seems like the cucumber gets parsed 3 times, by the old cucumber-core code, the messages code, and the cucumber-gherkin code, and the one it's using here is the only one that doesn't have the expanded docstring text.

@aurelien-reeves
Copy link
Contributor

aurelien-reeves commented Nov 5, 2021

@aurelien-reeves Something else I noticed, the JSON formatter no longer has expanded docstrings for Scenario Outlines. Is there a reason to use the step_source from the lookup here rather than the multiline_arg from the test_step like it used to. https://github.com/cucumber/cucumber-ruby/blob/main/lib/cucumber/formatter/json.rb#L178

It seems like the cucumber gets parsed 3 times, by the old cucumber-core code, the messages code, and the cucumber-gherkin code, and the one it's using here is the only one that doesn't have the expanded docstring text.

Well, this is beyond my knowledge actually 😅
@aslakhellesoy @luke-hill @mattwynne any idea?

@aslakhellesoy
Copy link
Contributor

Sorry, I haven't made any significant changes to this file for over a decade.

@eraffel-MDSol
Copy link
Member Author

I guess I'll ping @brasmusson since he made the most recent change to the relevant code, and otherwise I'll update it to what I said, using the property from test_step?

@aurelien-reeves
Copy link
Contributor

I guess I'll ping @brasmusson since he made the most recent change to the relevant code, and otherwise I'll update it to what I said, using the property from test_step?

Don't hesitate to submit PRs 👍

@eraffel-MDSol
Copy link
Member Author

My hesitation is that it'd be reverting a change, and I don't understand why the change was made in the first place, lol.

@aurelien-reeves
Copy link
Contributor

If some tests are broken, we will have the opportunity to dig deeper and discuss it 😃
If not, the PR will have the opportunity to be reviewed and merged 🎉

Please note that we won't maintain the JSON formatter our-self - the mainteners - anymore. We won't deprecate and remove it either because it is used a lot, but we want to promote the messages formatter instead.

That said, we won't reject contributions to the JSON formatter. Of course we may have some feedback depending on the impact those contributions may have, but they will always be welcome.

@aurelien-reeves
Copy link
Contributor

@eraffel-MDSol do you want to do something else with that PR? Or is it ready to be merged?

@eraffel-MDSol
Copy link
Member Author

Yea, I still want to fix the issue with placeholders in doc strings. I'll try to get to it this week. Just been busy. Thanks!

@aurelien-reeves
Copy link
Contributor

No worries, no rush :)

@eraffel-MDSol
Copy link
Member Author

@aurelien-reeves I pushed my changes to support doc strings in Scenario Outlines

@aurelien-reeves
Copy link
Contributor

@eraffel-MDSol it looks good, thanks 👍
Just missing an entry in the changelog for the docstring support in scenario outlines, and it should be fine 👌

@aurelien-reeves aurelien-reeves enabled auto-merge (squash) December 7, 2021 07:25
@aurelien-reeves
Copy link
Contributor

Thanks a lot @eraffel-MDSol 🤩

@aurelien-reeves aurelien-reeves merged commit e502ace into cucumber:main Dec 7, 2021
@aslakhellesoy
Copy link
Contributor

Hi @eraffel-MDSol,

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:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

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

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.

3 participants