Remove dependencies if not needed #138

NelsonVides · 2024-08-27T19:22:40Z

Just trying to minimise the amount of code and dependencies a project gets, OTP covers RFC3339 since 21.0, and json since 27.0. I'd like to slim of dependencies my deployments and OTP's libraries have better support (and performance).

Edit: just wrapped the logic around a OTP_RELEASE macro, that way we can keep backwards compatibility.

andreineculau · 2024-08-28T07:09:45Z

Fantastic Nelson!
I'll have a look

PS: backwards compatibility for OSS is more of a black box but thanks to the setup we have is fairly smooth

NelsonVides · 2024-08-29T16:19:34Z

Hey there :) Any updates on this one? Just asking for a timeline, no pressure intended 🥲

zmstone

I'm not maintainer, but great job! Thank you!

zmstone · 2024-08-29T17:35:51Z

Unsure why github actions are not triggered.

zmstone · 2024-08-29T17:37:58Z

src/jesse.app.src

@@ -11,8 +11,6 @@
                   , public_key
                   , ssl
                   , inets
-                   , jsx


nit: one can actually add .app.src.script, see https://rebar3.org/docs/configuration/config_script

Yeah, but we don't actually need jsx here anyways, this list is for apps to be started, jsx is not an app but only code to be loaded, that rebar will put in the right path for the BEAM to load the code automatically

it's also for rebar3 release to find the apps to be copied.

Absolutely very good point I don't know how could I forget! Fixed! :)

andreineculau · 2024-08-29T20:42:08Z

Unsure why github actions are not triggered.

First time contributors. Need to click Checks > CI > Approve and Run.

https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks

andreineculau · 2024-08-29T20:44:15Z

src/jesse_cli.erl

@@ -90,7 +90,7 @@ jesse_run(JsonInstance, Schema, Schemata) ->
  {ok, _} = application:ensure_all_started(jesse),
  ok = add_schemata(Schemata),
  {ok, JsonInstanceBinary} = file:read_file(JsonInstance),
-  JsonInstanceJsx = jsx:decode(JsonInstanceBinary, [{return_maps, false}]),


It might be an ok change, and I don't have all the details in my head at this very moment, but why did you decide to remove return_maps=false ?

Since there's the "no macros in dynamic calls" to fix (see the CI run), and I guess the solution will be to create a function for encapsulation, maybe we just keep the return_maps=false

I removed it because json does not support it, and I followed the logic and manually verified it is not needed (anymore) 🤔

That CI error, huh, elvis. Will fix 👌🏽

Pushed a fix :)

Sorry, made a typo on the fix, pushed again 😅

NelsonVides · 2024-09-03T08:36:51Z

@andreineculau any updates on this one? 😇

andreineculau

Thank you for your patience and for the reminder.
CI is green 💪
I left some suggestions which are just the beard talking. I wait briefly for your reply, and then merge and schedule a version

Thank YOU!

andreineculau · 2024-09-03T15:12:53Z

rebar.config

@@ -1,5 +1,5 @@
 %%-*- mode: erlang -*-
-{profiles, [{test, [{deps, [{ proper, "1.4.0"}]}]}]}.
+{profiles, [{test, [{deps, [{jsx, "3.1.0"}, {proper, "1.4.0"}]}]}]}.


Suggested change

{profiles, [{test, [{deps, [{jsx, "3.1.0"}, {proper, "1.4.0"}]}]}]}.

{profiles, [{test, [{deps, [{jsx, "3.1.0"}, {rfc3339, "0.9.0"}, {proper, "1.4.0"}]}]}]}.

andreineculau · 2024-09-03T15:14:18Z

src/jesse_lib.erl

+  -else.
+  %% OTP 26 to 21.
+json_decode(Bin) ->
+    jsx:decode(Bin).


Suggested change

jsx:decode(Bin).

jsx:decode(Bin, [{return_maps, false}).

I'm only afraid this will have suboptimal performance, but well, I guess that if performance is critical it can run an upgrade to OTP27, jsx was lagging behind long ago anyway :)

andreineculau · 2024-09-03T15:14:44Z

src/jesse_lib.erl

+-else.
+  %% OTP 20 or lower.
+json_decode(Bin) ->
+    jsx:decode(Bin).


Suggested change

jsx:decode(Bin).

jsx:decode(Bin, [{return_maps, false}).

NelsonVides · 2024-09-03T15:36:06Z

Thank you for your patience and for the reminder. CI is green 💪 I left some suggestions which are just the beard talking. I wait briefly for your reply, and then merge and schedule a version

Thank YOU!

Comments applied 💪🏽

Looking forward for a new version! 🙃

andreineculau · 2024-09-03T16:21:54Z

Published. Hope everything still works >_<

Remove unneeded dependency, OTP covers since 21.0

bdcdab1

NelsonVides changed the title ~~Remove rfc3339~~ Remove dependencies if not needed Aug 28, 2024

NelsonVides added 2 commits August 28, 2024 07:58

Remain backwards-compatible with older OTP

72a1da9

Use json from OTP27 and jsx optionally otherwise

9a39bf2

NelsonVides force-pushed the slim_dependencies branch from adcd8e2 to 9a39bf2 Compare August 28, 2024 05:59

zmstone approved these changes Aug 29, 2024

View reviewed changes

zmstone reviewed Aug 29, 2024

View reviewed changes

andreineculau reviewed Aug 29, 2024

View reviewed changes

NelsonVides force-pushed the slim_dependencies branch from 0cc391a to 238df4f Compare August 30, 2024 06:50

NelsonVides added 2 commits August 31, 2024 13:36

Optionally include jsx and rfc3339 in releases

9e8fa79

Encapsulate json module (elvis error)

8b0daa2

NelsonVides force-pushed the slim_dependencies branch from 238df4f to 8b0daa2 Compare August 31, 2024 11:37

NelsonVides requested a review from andreineculau September 2, 2024 07:49

andreineculau approved these changes Sep 3, 2024

View reviewed changes

Apply review comments

76345bc

andreineculau merged commit 37fee73 into for-GET:master Sep 3, 2024
21 checks passed

NelsonVides deleted the slim_dependencies branch September 3, 2024 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependencies if not needed #138

Remove dependencies if not needed #138

NelsonVides commented Aug 27, 2024 •

edited

Loading

andreineculau commented Aug 28, 2024

NelsonVides commented Aug 29, 2024

zmstone left a comment •

edited

Loading

zmstone commented Aug 29, 2024

zmstone Aug 29, 2024

NelsonVides Aug 29, 2024

zmstone Aug 29, 2024

NelsonVides Aug 29, 2024

andreineculau commented Aug 29, 2024

andreineculau Aug 29, 2024

andreineculau Aug 29, 2024

NelsonVides Aug 29, 2024

NelsonVides Aug 29, 2024

NelsonVides Aug 30, 2024

NelsonVides commented Sep 3, 2024

andreineculau left a comment

andreineculau Sep 3, 2024

andreineculau Sep 3, 2024

NelsonVides Sep 3, 2024

andreineculau Sep 3, 2024

NelsonVides commented Sep 3, 2024

andreineculau commented Sep 3, 2024

	{profiles, [{test, [{deps, [{jsx, "3.1.0"}, {proper, "1.4.0"}]}]}]}.
	{profiles, [{test, [{deps, [{jsx, "3.1.0"}, {rfc3339, "0.9.0"}, {proper, "1.4.0"}]}]}]}.

Remove dependencies if not needed #138

Remove dependencies if not needed #138

Conversation

NelsonVides commented Aug 27, 2024 • edited Loading

andreineculau commented Aug 28, 2024

NelsonVides commented Aug 29, 2024

zmstone left a comment • edited Loading

Choose a reason for hiding this comment

zmstone commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreineculau commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides commented Sep 3, 2024

andreineculau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides commented Sep 3, 2024

andreineculau commented Sep 3, 2024

NelsonVides commented Aug 27, 2024 •

edited

Loading

zmstone left a comment •

edited

Loading