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

Adding time extensions #11

Closed
wants to merge 2 commits into from
Closed

Adding time extensions #11

wants to merge 2 commits into from

Conversation

shinayser
Copy link
Contributor

@shinayser shinayser commented Oct 28, 2019

Hello!

I got the code produced by jogboms on https://github.com/jogboms/time.dart and merged his great libraries into dartx.

This commit introduces all the jogbom's extensions to time types, like producing durations through integers and some other utility methods.

@simc
Copy link
Owner

simc commented Oct 28, 2019

Thanks for your contribution!

The .seconds, .milliseconds, etc. getters are already defined on num so they will work on int and double (Source).

The extensions on Date and Duration are new and I will merge them if you remove the int extensions.

@shinayser
Copy link
Contributor Author

shinayser commented Oct 28, 2019

Wow great! I haven't seen those members. Nice job then =)

@jogboms
Copy link

jogboms commented Oct 28, 2019

Hey @shinayser @leisim

By the way, amazing idea to curate these extensions but I have a slight tingle in my stomach that copying source code and tests may not be an elegant solution as both libraries could take different directions and divert at some point from bug fixes, api changes and just normal improvements and one would have to track both implementations and test cases whenever this happens.

@leisim with your permission, the improvements on Time.dart to support both variants of num would indeed be a good idea and both libraries could exists as different entities. What do you think?

@shinayser
Copy link
Contributor Author

Just removed the duplicated int extensions!

@simc
Copy link
Owner

simc commented Oct 28, 2019

@jogboms

I have a slight tingle in my stomach that copying source code and tests may not be an elegant solution.

I agree. I would never copy your without asking. I actually took this functionality from Kotlin before your package was released.

with your permission, the improvements on Time.dart to support both variants of num would indeed be a good idea and both libraries could exists as different entities. What do you think?

Yeah sure, no problem. Do you think I should remove all time related getters? My vision with dartx was to create a single library which contains every extension you could dream of because with Kotlin I always had to use multiple libraries with overlapping functionality and that was annoying and created name conflicts.

@shinayser
Copy link
Contributor Author

My objective of doing the merge was to make dartx the only library needed for all this small extensions that we use all the time on our work days.
In my opinion, one library with all extensions is much better than multiple multiple libraries.

Imagine if you want to use timer extensions, you will have to import it manually.
Than, if you want to use another extensions library, you will also have to import it manually.
See the problem? Dartx fix it by doing just one import line.

@jogboms
Copy link

jogboms commented Oct 28, 2019

I agree. I would never copy your without asking. I actually took this functionality from Kotlin before your package was released.

No issues really, I was just more worried on how to proceed when things start to diverge.

Do you think I should remove all time related getters?

If you wouldn't mind, I could make Time.dart more robust with those inclusions.

Kotlin I always had to use multiple libraries with overlapping functionality and that was annoying and created name conflicts.

At least with the current implementation of extensions for dart we shouldn't be worried about this, There is an open discussion about global scoping (even though I wouldn't want that to be honest) and that could break projects down the road for every one.

@rrousselGit
Copy link
Contributor

rrousselGit commented Oct 28, 2019

A more elegant solution is to have dartx depend on @jogboms's solution. Then use the export directive to expose them.

@simc
Copy link
Owner

simc commented Oct 28, 2019

Shit sorry I edited you comment...

@simc
Copy link
Owner

simc commented Oct 28, 2019

A more elegant solution is to have dartx depend on @jogboms's solution. Then use the export directive to expose them.

I didn't even think of that. Great idea. I'll do that if @jogboms agrees 👍

@jogboms
Copy link

jogboms commented Oct 28, 2019

@rrousselGit good of you to join in :)

In my opinion, one library with all extensions is much better than multiple multiple libraries.

@shinayser, considering the fact that extensions are scoped to their imports, I would not think this is the way to go.

@jogboms
Copy link

jogboms commented Oct 28, 2019

@leisim I was already edging towards that idea in my mind. Good to know we are all in sync 💙 👍

@shinayser
Copy link
Contributor Author

shinayser commented Oct 28, 2019

Awesome. So, making dartx depend on time will be the best sollution.

Will you do this @leisim, or you want me to push that change?

@simc
Copy link
Owner

simc commented Oct 28, 2019

@leisim I was already edging towards that idea in my mind. Good to know we are all in sync 💙 👍

Perfect, let's do that. You create extensions for num, right?

@simc
Copy link
Owner

simc commented Oct 28, 2019

Awesome. So, making dartx depend on dart will be the best sollution.

This is also a future proof solution to solve these kinds of conflict.

@jogboms
Copy link

jogboms commented Oct 28, 2019

Yes @leisim will do that and add more tests.

@shinayser
Copy link
Contributor Author

Great then. So, this issue is closed. Thanks @leisim and @jogboms.

Let's keep dart rocking :)

@shinayser shinayser closed this Oct 28, 2019
@rrousselGit
Copy link
Contributor

That was a nice smooth discussion! Epic ❤️

@simc
Copy link
Owner

simc commented Oct 28, 2019

@jogboms @rrousselGit @shinayser

I made the changes and added a small example to the readme. A new version will be published once time.dart supports num.

@shinayser
Copy link
Contributor Author

I made the changes and added a small example to the readme. A new version will be published once time.dart supports num.

Amazing! Can't wait to use it with Flutter! Anyone got any idea when we getting dart 2.6 on it?

@jogboms
Copy link

jogboms commented Oct 29, 2019

@shinayser not so sure on the timeline.

@leisim published v1.1.0 of Time.dart

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.

None yet

4 participants