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

Move to core? #8

Closed
natefaubion opened this issue Dec 18, 2017 · 14 comments
Closed

Move to core? #8

natefaubion opened this issue Dec 18, 2017 · 14 comments

Comments

@natefaubion
Copy link

IMO, this is an essential library that I think should belong in core. Would you be open to donating it?

/cc @hdgarrood @garyb @LiamGoodacre

@sharkdp
Copy link
Collaborator

sharkdp commented Dec 19, 2017

Absolutely. Let me know what needs to be done in order to realize this.

@hdgarrood
Copy link
Collaborator

Actually, on second thoughts, adding libraries to core complicates releases, and this library seems to be doing ok where it is now; unless we ever want to depend on this library in any other core library, I think it might be best to leave this as it is.

@hdgarrood
Copy link
Collaborator

Past me is talking nonsense, this should clearly be in core. I will poke around a bit and propose a way forward.

@hdgarrood
Copy link
Collaborator

The only barrier I can see is that core libraries ought not to depend on non-core libraries, even for their devDependencies - this rules out test-unit. However I think it should be relatively easy to rewrite the tests to depend on assert instead. It will admittedly make the test output worse, but I imagine these tests aren't failing (or even being run) very often anyway.

@JordanMartinez
Copy link
Contributor

I've submitted #10 to migrate tests to assert. What else do we need to do to move this to core? Or is it just lack of time to do the work?

@hdgarrood
Copy link
Collaborator

IIRC you can’t transfer repos to an org unless you have the ability to create repos in that org, so I’d suggest:

  • @sharkdp transfers this repo to a core team member’s personal account (I’m happy to do this)
  • that core team member transfers the repo to the purescript org,
  • that core team member adds @sharkdp with admin access to the purescript-numbers repo in its new location.

I think this can happen either before or after #10 is merged.

@sharkdp
Copy link
Collaborator

sharkdp commented Oct 22, 2020

Happy to do this, but will take a few days, as I am currently on vacation

@hdgarrood
Copy link
Collaborator

Great! No rush at all :)

@sharkdp
Copy link
Collaborator

sharkdp commented Oct 23, 2020

I requested the move to your account.

I'd be glad if you could handle #10 once the repo has been moved (I would hope that the PR will be migrated).

I don't need admin access on the new repository.

Maybe one last thing from my side: I'm not sure if (all of) Data.Number.Approximate should go into core. It seems a bit too "opinionated" for a core library.

@hdgarrood
Copy link
Collaborator

Thanks! I see what you mean about it being opinionated, but at the same time I think the API it provides does make sense and I think changing or removing it would likely cause more of a problem for people who are using that module, so I'm happy to leave it as-is personally.

@JordanMartinez
Copy link
Contributor

Thanks sharkdp! (not tagging you in case you're still on vacation)

Can this issue be closed then? URL shows that it's under purescript now.

I think changing or removing it would likely cause more of a problem for people who are using that module, so I'm happy to leave it as-is personally.

Would we ever consider porting just that content to its own separate repo, even if it's still in core? Or should that be reconsidered in the future when we do breaking changes again so that we can get v0.14.0 sooner?

@hdgarrood
Copy link
Collaborator

Yes, thanks, I’ll close it now.

We can consider what to do with Data.Number.Approximate now, but for me it’s not a question of now versus later, I just think it’s a useful API, and in my view it’s not really that opinionated, so I don’t think including it in core should be considered a problem. Comparing numbers is a pretty common thing to want to do.

@natefaubion
Copy link
Author

To me the only thing opinionated about it is unicode operators, which we have a policy against in core.

@hdgarrood
Copy link
Collaborator

Yeah, although for that I wouldn’t mind making an exception in this case, since removing them would be a breaking change.

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

4 participants