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

Upgrating #1

Merged
merged 4 commits into from
May 13, 2021
Merged

Upgrating #1

merged 4 commits into from
May 13, 2021

Conversation

vkatsuba
Copy link
Owner

@vkatsuba
Copy link
Owner Author

Hi @manuel-rubio,

I'm impressed with your upgrade, if you don't mind I would like to merge with it and release a new version(2.0.0) of bellboy based on your latest changes.

Regards,
--V

@vkatsuba
Copy link
Owner Author

@manuel-rubio can I merge it or you want add other changes in the scope of current PR?

Regards,
--V

@manuel-rubio
Copy link
Contributor

Hi @vkatsuba I still didn't finish it, but yes, you can create a PR and merge when you want. Thanks.

@manuel-rubio
Copy link
Contributor

@manuel-rubio can I merge it or you want add other changes in the scope of current PR?

@vkatsuba I'll let you know when I finish :-)

@vkatsuba
Copy link
Owner Author

Hi @vkatsuba I still didn't finish it, but yes, you can create a PR and merge when you want. Thanks.

Great, many thanks! I will merge it and also provide CI for more comfortable sync. If you will have any other ideas of improvements, please feel free create PR.

Regards,
--V

@vkatsuba vkatsuba merged commit 2f1e01a into vkatsuba:master May 13, 2021
@manuel-rubio
Copy link
Contributor

@vkatsuba ok, at the moment I'm going to leave it as is. I want to create an abstraction layer to use the same values to trigger an SMS and returning common values (in addition to the specific ones), I want to put the sensitive data for each provider (auth, keys, sid, ...) into the configuration and increase the test coverage.

@vkatsuba
Copy link
Owner Author

@vkatsuba ok, at the moment I'm going to leave it as is. I want to create an abstraction layer to use the same values to trigger an SMS and returning common values (in addition to the specific ones), I want to put the sensitive data for each provider (auth, keys, sid, ...) into the configuration and increase the test coverage.

Got it, thanks for updates. So, as I understand we can create 2.0.0 version and if you will have any changes we will include it into the minor versions - sounds as plan? And before create new release, can I ask you - why you was start use jsone instead of jsx?(I have nothing against it, I just wanted to hear your opinion).

Regards,
--V

@vkatsuba
Copy link
Owner Author

@manuel-rubio also please notes that was added GitHub Actions with otp: [21.3, 22.0.7, 22.3, 23.0.4, 23.2.7.0, 24.0], so, you can rebase your fork and run CI in your repo.

@manuel-rubio
Copy link
Contributor

Got it, thanks for updates. So, as I understand we can create 2.0.0 version and if you will have any changes we will include it into the minor versions - sounds as plan?

Yes!

And before create new release, can I ask you - why you was start use jsone instead of jsx?(I have nothing against it, I just wanted to hear your opinion).

My main concern is use 100% Erlang libraries to avoid incompatibilities so, jiffy never was an option, and jsx never liked me too much, based on benchmarks, you can see jsone is better and I like more how it handle JSON:

Non HiPE:

jiffy jsone poison jazz jsx
maps 7.23 μs/op 10.64 μs/op (2) 13.58 μs/op 19.30 μs/op 29.28 μs/op
lists 210.40 μs/op 157.39 μs/op (3) 109.30 μs/op 201.82 μs/op 357.25 μs/op
strings* 98.80 μs/op 595.63 μs/op (5) 416.78 μs/op 399.89 μs/op 262.18 μs/op
string escaping* 144.01 μs/op 732.44 μs/op (2) 1318.82 μs/op 1197.06 μs/op 1324.04 μs/op
large value** 408.03 μs/op 1556.85 μs/op (3) 1447.71 μs/op 1824.05 μs/op 2184.59 μs/op
pretty print** 420.94 μs/op 1686.55 μs/op (3) 1534.74 μs/op 2041.22 μs/op 5533.04 μs/op

HiPE:

jiffy jsone poison jazz jsx
maps 7.69 μs/op 6.12 μs/op (1) 12.32 μs/op 22.90 μs/op 27.03 μs/op
lists 207.75 μs/op 69.93 μs/op (1) 79.04 μs/op 229.95 μs/op 278.01 μs/op
strings* 96.67 μs/op 321.69 μs/op (5) 142.43 μs/op 310.10 μs/op 179.96 μs/op
string escaping* 146.85 μs/op 317.10 μs/op (2) 1277.54 μs/op 1311.85 μs/op 767.67 μs/op
large value** 409.73 μs/op 664.34 μs/op (2) 806.24 μs/op 1630.21 μs/op 1777.62 μs/op
pretty print** 419.55 μs/op 724.28 μs/op (2) 844.76 μs/op 1888.71 μs/op 4872.34 μs/op

* binary representation of UTF-8-demo.txt

** generated.json

@vkatsuba
Copy link
Owner Author

Got it, thanks. Latest updates:

Thanks again for contribution!

Regards,
--V

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.

2 participants