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

Consider linking project fork: greatcloak/decimal #324

Open
epelc opened this issue Jul 25, 2023 · 14 comments
Open

Consider linking project fork: greatcloak/decimal #324

epelc opened this issue Jul 25, 2023 · 14 comments

Comments

@epelc
Copy link

epelc commented Jul 25, 2023

Hello,

Not sure if anyone is still around to add a link to readme. We just started a fork after using decimal for a long while.

Sadly it seems shopspring was bought out and no longer maintains this repo. Our company develops ecommerce and shipping software and is starting a fork. We use this project as part of our billing system so have a vested interesting in maintaining it and fixing any bugs.

We've just launched version 1.4.1 which attempts to cleanup the project, fix linter warnings, reorganize/split files into more readable ones, etc. We've also added BSON support and made it easier to work with 0 values for JSON by accepting an empty string when unmarshalling.

https://github.com/greatcloak/decimal

Our future plans are to look at PRs for bug fixes such as #322, #301, and others. We are a bit more open about adding 3rd party deps where it makes sense. However we are using this in several products so also value non breaking changes and compatibility much like shopspring.

@temuera
Copy link

temuera commented Jul 31, 2023

this is a good news!

@varfrog
Copy link

varfrog commented Sep 5, 2023

Good news, so cool to see abandoned projects revived. Could you please add the latest release tag in that new repo? Currently it shows the number of tags but not the latest release

@epelc
Copy link
Author

epelc commented Sep 5, 2023

@varfrog here you go https://github.com/greatcloak/decimal/releases/tag/v1.4.1

@harryzcy
Copy link

@epelc Can you please enable issues on your repo?

@epelc
Copy link
Author

epelc commented Sep 17, 2023

It is enabled now

@mwoss
Copy link
Member

mwoss commented Dec 30, 2023

Hi all! Sorry for the late reply :<, I was busy working on other projects and was not able to take care of this library. Priorities changed now and I should be able to put some time and love into this library :3 (at least to fix some crucial bugs)

I can mention your project in the documentation but I want to understand what's your vision on this fork. I saw the project was 8 commits ahead, but most of them were about file reorganization and linting. The only new thing is support for BSON marshaling that we don't want to implement as we are aiming for a zero-dependency library - see #92 and #168. Are you aiming to support multiple different DB drivers? What would be the main difference between this library and your fork?

@epelc
Copy link
Author

epelc commented Dec 31, 2023

Hi @mwoss thanks for getting back. Curious what you are working on now. Sounds exciting! Thanks for spurring some action here as well I've just applied a couple bug fixes for open PRs here

Anyways we will keep our fork going since many of our projects depend on it and our main priority is to make web application development safer/easier.

Our main priorities:

  • Ease of use in web applications(re json defaults, provide or auto register common DB driver marshalers/unmarshalers, etc)
  • Backed by a company again(we build ecommerce, shipping, and accounting related software all in go for the past 10 years).
    • It looks like Spring was acquired a few years ago and at some point ShopRunner deallocated resources from this or perhaps stopped using the project internally?
  • Fixing/accepting bugs and performance improvements

I agree with you that keeping the library dependency free is a major positive. However I think there can be a caveat that core features/majority of the library is low/dependency free while sprinkling in some dependencies where appropriate is helpful. In this case making the Decimal data type work with common databases by default is very helpful in regular apps and tests.

Auto register/builtin marshalers/unmarshalers vs explicit setup

One could argue that the decimal_bson.go could be moved to a subpackage and even a separate go module. However this would require users to manually call RegisterBSONDecimalCodec in all projects. The more important issue is that you also have to do this in all tests as well which is the more common place that people forget.

@mwoss
Copy link
Member

mwoss commented Dec 31, 2023

Thanks for such a detailed rationale @epelc :3 So the main difference would be a support for various DB drivers that could allow easier web application development, and the company support itself. Makes sense to me :3
The last bullet point applies to both this library and your fork in my opinion (at least right now).

I would do a small research after New Year's Day and gather all forks and library alternatives that seem like good candidates to be mentioned in the README. I'll let you know when PR with this change will be ready. Have a great new year and new eve! :D

@mwoss
Copy link
Member

mwoss commented Apr 7, 2024

Hi @epelc! I'm sorry it took me so long to do this, I had a lot of things on my head in the last few months. Here's the PR #363 that introduces a new section to the docs (alternative libraries) when I briefly mention your fork and other similar libraries. Please take a look if this is fine for you :D

One more small thing. I saw that instead of cherry-picking you copy pasted a few commits to your fork. I would highly appreciate it if in the future you would only use git cherry-pick to keep the fork in sync with the origin repo. This repository is under the MIT license and you can do whatever you want with it, but I believe original authors would be happy if they get some credits for their work. I hope you understand my point of view :D

@epelc
Copy link
Author

epelc commented Apr 7, 2024

Thanks @mwoss! Appreciate the link and will try to git cherry-pick in the future. Sorry about that was a new workflow. It was my intent to keep their credits and authorship.

I think I failed originally on the first couple merges but got it right on the two latest ones. I'll definitely confirm going forward.

image

New w/ credits on later ones. I need to look in going forward git cherry-pick though as I'm not 100% this keeps the forks in sync like you said even though it has attribution working. I'll double check next time I merge in any changes.
image

I looked at the PR and it looks good. Seems like we are the primary fork focusing on keeping the library's api as close to as it was. We are focusing on keeping arbitrary precision support as our use cases are billing and ecommerce related web applications. Anything good for web apps and websocket/rest json apis is a plus for us. Long term stability and bug fixes are also a major priority due to our use in several large applications.

@epelc
Copy link
Author

epelc commented Apr 8, 2024

@mwoss released an update w/ latest perf and bug fixes using git cherry-pick. Thanks for the info about git!

greatcloak/decimal@v1.4.3...v1.4.4

@mwoss
Copy link
Member

mwoss commented Apr 8, 2024

Thanks for doing that, I greatly appreciate that :D

Should I add mainly focused on solving billing and e-commerce related problems next to your library in mentioned PR? What do you thin?

@epelc
Copy link
Author

epelc commented Apr 8, 2024

How about something like this? I think web applications is a good keyword since that is a common use case people are looking for and is what we are primarily using for.

fork of this library, with out-of-the-box BSON marshaling support -> fork focusing on billing and e-commerce web application related use cases. Includes out-of-the-box BSON marshaling support

@mwoss
Copy link
Member

mwoss commented Apr 8, 2024

Sounds great to me, I will use that :3 Thanks!

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

No branches or pull requests

5 participants