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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃Ч Remove MultiJSON, JSON and Yajl (Use Oj instead) #3785

Merged
merged 4 commits into from May 23, 2024

Conversation

philippthun
Copy link
Member

@philippthun philippthun commented May 7, 2024

  • Use Oj.load and Oj.dump
  • Oj.load => rescue StandardError
  • Oj.dump + RestController => mode: :compat
  • No pretty option anymore
  • Add Cop that detects the usage of other JSON libraries than Oj
  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@philippthun philippthun force-pushed the remove-multi-json branch 3 times, most recently from 2ef4b92 to dea1f84 Compare May 8, 2024 10:11
@philippthun philippthun changed the title wip Remove MultiJSON May 8, 2024
- Use Oj.load and Oj.dump
- Oj.load => rescue StandardError
- Oj.dump + RestController => mode: :compat
- No 'pretty' option anymore
@philippthun philippthun changed the title Remove MultiJSON Remove MultiJSON and JSON May 8, 2024
@philippthun philippthun changed the title Remove MultiJSON and JSON Remove MultiJSON and JSON (Use Oj instead) May 8, 2024
@philippthun philippthun changed the title Remove MultiJSON and JSON (Use Oj instead) 馃Ч Remove MultiJSON and JSON (Use Oj instead) May 8, 2024
- Use Oj.load and Oj.dump
@philippthun philippthun changed the title 馃Ч Remove MultiJSON and JSON (Use Oj instead) 馃Ч Remove MultiJSON, JSON and Yajl (Use Oj instead) May 8, 2024
- Use Oj.load and Oj.dump
@philippthun philippthun marked this pull request as ready for review May 14, 2024 10:27
@philippthun
Copy link
Member Author

CATS have been successfully executed (same configuration as asha and scar environments).

Copy link
Contributor

@svkrieger svkrieger left a comment

Choose a reason for hiding this comment

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

Overall a very nice change which tidies up our codebase.

Sometimes you removed pretty and added mode: :compat, sometimes not. Is this on purpose? I added comments to some places where I saw that mode is not specified. Later I saw this is in many more places, so probably not an issue if it's not needed.

Very cool that you added a Rubocop rule!

app/controllers/v3/root_controller.rb Show resolved Hide resolved
lib/cloud_controller/diagnostics.rb Show resolved Hide resolved
spec/api/documentation/apps_api_spec.rb Show resolved Hide resolved
Copy link
Member

@moleske moleske left a comment

Choose a reason for hiding this comment

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

I will admit I did not look at every individual file. Very excited to remove a dependency from the gemfile!

@philippthun philippthun merged commit f122c77 into cloudfoundry:main May 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants