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

debt: delete json formatter #1916

Merged
merged 8 commits into from
May 30, 2022
Merged

debt: delete json formatter #1916

merged 8 commits into from
May 30, 2022

Conversation

davidjgoss
Copy link
Contributor

Summary

Delete json-formatter and its references and usages in the monorepo.

Motivation and Context

Fixes #1915.

How Has This Been Tested?

If the build passes, we're good.

@davidjgoss davidjgoss changed the title Delete json formatter debt: delete json formatter Mar 2, 2022
@davidjgoss davidjgoss marked this pull request as ready for review March 2, 2022 20:39
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.

I would still wait a little bit before removing the standalone json-formatter:

  • it may be used by users so we have to keep a safe place for the binaries, and announce and document the removal
  • some other packages tests still rely on it to generate some test data

@davidjgoss
Copy link
Contributor Author

it may be used by users so we have to keep a safe place for the binaries, and announce and document the removal

Good point, perhaps we should break it out to a repo and then archive that before deleting here?

some other packages tests still rely on it to generate some test data

The only one I found was json-to-messages and I've refactored that dependency out; everything's green. Are there more I'm not seeing?

@aurelien-reeves
Copy link
Contributor

Good point, perhaps we should break it out to a repo and then archive that before deleting here?

Yes, I would go for something like this.

The only one I found was json-to-messages and I've refactored that dependency out; everything's green. Are there more I'm not seeing?

Oh, well done! Sorry, I did not get that 😅
So, as long as the CI is green and not impacted anymore with this, I am ok with that point 👍

@davidjgoss
Copy link
Contributor Author

I've created a repo which has the contents of json-formatter including the history https://github.com/cucumber/json-formatter

It would probably need a little work to make it operational as a standalone package - is that worthwhile though?

@aurelien-reeves
Copy link
Contributor

I've created a repo which has the contents of json-formatter including the history https://github.com/cucumber/json-formatter

It would probably need a little work to make it operational as a standalone package - is that worthwhile though?

Definitely, yes! Thank you! 🤩

@aslakhellesoy aslakhellesoy added the 🏦 debt Tech debt label Apr 21, 2022
@badeball
Copy link
Member

badeball commented May 9, 2022

I'm a bit out of the loop regarding JSON formatting and was wondering if someone could help me catch up. I've authored / authoring a plugin for Cypress to allow users to write Gherkin specs, a library which re-implements much of the behavior users are familiar with from Cucumber.

JSON reports are fairly popular and I've refactored my implementation to output messages, because that was significantly easier implementation wise, and I rely on users to use https://github.com/cucumber/json-formatter to further convert this output to JSON. Is this stand-alone formatter about to go away?

@mattwynne
Copy link
Member

mattwynne commented May 9, 2022

Hey @badeball, thanks for getting in touch and thanks for all your hard work to bring Cucumber functionality to the Cypress community!

We originally had a vision whereby we would remove the language-specific JSON formatters from all the Cucumber implementations get people to move over to using the new NDJSON message protocol instead. We wrote this Go binary as part of that strategy so that there was a fall-back, but we got quite a bit of pushback about this, and decided to keep the native JSON formatters for now. We've had similar issue to you with people getting false positives from their anti-virus scanners from the Go binary.

We weren't sure if anyone else was using this tool so as we work to break up this monorepo we'd thought it was OK to just remove it and archive the code. Since we now know there are people relying on it we'll make sure we keep the code live, but it will start to live in this new stand-alone repo once this PR is merged.

@davidjgoss
Copy link
Contributor Author

Okay, thanks to @mattwynne's efforts on cucumber/json-formatter#1 we now have the JSON formatter in its own repo with a green build, so this should be good to go.

@mattwynne
Copy link
Member

mattwynne commented May 25, 2022

Okay, thanks to @mattwynne's efforts on cucumber/json-formatter#1 we now have the JSON formatter in its own repo with a green build, so this should be good to go.

I'd just like to finish the work on cucumber/json-formatter#6 before we call this done, so that we can point everyone over to the new repo for downloading the binaries.

I'd also like to know if there are any docs anywhere that we need to update. @badeball are you aware of any signposts in the wild that will need to be updated?

@mattwynne
Copy link
Member

OK, with cucumber/json-formatter#6 and binaries available to download from that repo, I think we're good to go with this.

I wonder what we should do about the releases sitting on this repo. I've updated the latest release to point to the new repo. Should we delete the assets from that release too, for clarity? What about older releases?

@davidjgoss
Copy link
Contributor Author

Going to merge this once the current run is green, as it keeps getting conflicted.

@davidjgoss davidjgoss merged commit 7ea60a7 into main May 30, 2022
@davidjgoss davidjgoss deleted the delete-json-formatter branch May 30, 2022 06:55
@mattwynne
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏦 debt Tech debt
Projects
Development

Successfully merging this pull request may close these issues.

Remove standalone json formatter
5 participants