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

How about using NodaMoney instead of money-type-for-the-clr? #3

Open
jean opened this issue Jul 17, 2018 · 5 comments
Open

How about using NodaMoney instead of money-type-for-the-clr? #3

jean opened this issue Jul 17, 2018 · 5 comments

Comments

@jean
Copy link

jean commented Jul 17, 2018

How about using https://github.com/remyvd/NodaMoney vs https://bitbucket.org/rplaire/money-type-for-the-clr ?

@Piedone
Copy link
Member

Piedone commented Jul 17, 2018

Because the current one is already done :). At the time we first needed such a type basically only this implementation was available, and we continued to use it for this module too. If we'd start today probably the projects you mention would be better suitable.

But if you think one of the linked libraries are much better in certain aspects and you'd change the implementation then by all means you're welcome to do that and we'll check out your PR.

@jean
Copy link
Author

jean commented Jul 18, 2018

Ah, I didn't realize that NodaMoney came later.
I landed back here because we've just implemented our own money field! (As I'm sure many people have.)
And I'd like to avoid reinventing the wheel, and so I'm kicking your tyres 😉

What is the relation between Lombiq-Fields and https://github.com/Lombiq/Orchard-Abstractions ? Why only abstractions for Widgets and Parts, and not for Fields too? Does a QuickPart work with Lombiq.Fields.MoneyField (or Salween.Fields.MoneyField)?

Regarding a PR, I would prefer to work on a set of shared Fields, instead of creating another same-same-but-different module. So I will explore switching to your MoneyField.

But adding more fields to create a grab bag of unrelated fields doesn't sound appealing either, though splitting them out by Feature would make it better.

@Piedone
Copy link
Member

Piedone commented Jul 18, 2018

There is not much relation between this and the Abstractions module. However the latter one lets you create parts that'll then behave like standard parts; so you can e.g. add the Money Field onto a Quick Part as you'd do with any other part (or you can add it to the content type). Thus there's no incompatibility or anything.

We didn't create a Quick Field just because parts are the more complex things, and wanted to get that working first. No reason not to have a similar Quick Field implementation too, we just don't have time for it.

We sometimes need to find the balance between having a lot of tiny modules and having big utility modules with a lot of just loosely related features. This modules, while has fields that are independent of each other (the common thing being that they're all fields) is still quite small, so we didn't bother cutting it up. Also, each field has its own feature, so you can turn them on and off independently.

@jean
Copy link
Author

jean commented Jul 18, 2018

so you can e.g. add the Money Field onto a Quick Part as you'd do with any other part

Got it.

No reason not to have a similar Quick Field implementation too, we just don't have time for it.

We may be working towards a Quick Field implementation, I'll see if it's suitable for a PR.

balance between having a lot of tiny modules and having big utility modules

Agreed. Eventually I'd like to see results like django fields: many modules with fields grouped by topic or functionality, rather than company-specific surprise-packet utility bundles like Lombiq.Fields and Salween.Fields.

@Piedone
Copy link
Member

Piedone commented Jul 18, 2018

Thanks, looking forward to it.

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

2 participants