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

Add safe-coerce to packages.json and as dependency of newtype #787

Closed
wants to merge 2 commits into from

Conversation

fsoikin
Copy link
Contributor

@fsoikin fsoikin commented Dec 27, 2020

  • safe-coerce is present in src/groups/purescript.dhall, but not in packages.json.
  • newtype depends on safe-coerce, but this wasn't reflected in purescript.dhall, nor in packages.json

I bumped into this when working on preparing my own project for 0.14.
I asked a question about this on the forum and @hdgarrood suggested that a PR would be useful. So here I am.

"unsafe-coerce"
],
"repo": "https://github.com/purescript/purescript-safe-coerce.git",
"version": "master"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is "master", because that's how it exists in purescript.dhall

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, thank you!

@thomashoneyman
Copy link
Member

CI is failing because we need to drop the generics-rep dependency from a few more libraries -- generics-rep has been migrated to prelude. While you could go ahead and take care of that here, you certainly don't have to (we're working on all these issues right now).

@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 27, 2020

Oh, I can totally do that @thomashoneyman.
Just completely drop generics-rep, right?

@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 27, 2020

Well, ok, I've done that, but it's not that simple: generics-rep has some other stuff that's not just Generic itself - Show, Eq, etc.
I'm guessing there needs to be a PR to generics-rep repo itself, removing the Rep module, and then that version should be included in the set.
Right?

@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 27, 2020

Hmmm, but that doesn't work very well either: it seems it's no longer legal to derive instance foo :: Newtype Bar _, because the compiler still tries to generate implementations of wrap and unwrap, but they're no longer in the class. Yet, all the libraries still use that syntax.

I'm guessing the right solution is to have the compiler no longer generate the methods, which will make the old-style derive directive legal again. I just verified this locally: removing methods from Language.PureScript.Sugar.TypeClasses.Deriving::deriveNewtype makes generics-rep compile. I can push a PR to the compiler if you'd like. Unless I'm misunderstanding things again 😥

@JordanMartinez
Copy link
Contributor

@fsoikin See these PRs / issues:

Also, this PR is basically duplicating the work I was doing in #786. I didn't have the time today to finish working on it.

Lastly, I don't think we should update all packages in the package set. Rather, I think we should focus on just the core, contrib, web, and node libraries for the time being. I'd prefer to leave packages from other repos not yet updated until we are finished making our changes.

@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 27, 2020

Well, ok, it seems I've been too rash again. I'll close this out.

@fsoikin fsoikin closed this Dec 27, 2020
@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 27, 2020

Although I must say that the compiler is definitely still trying to implement the Newtype methods. Or am I missing a pending PR where that's also fixed?

@hdgarrood
Copy link
Contributor

I don’t think so, we do still need to do something about that.

@JordanMartinez
Copy link
Contributor

Well, ok, it seems I've been too rash again. I'll close this out.

I don't want to discourage you from contributing in the future. Thanks for helping us out. In this particular situation, I wasn't able to finish what I had started, nor give better guidance. I posted my comment above as a quick-and-dirty explanation for some of the things going on.

@fsoikin
Copy link
Contributor Author

fsoikin commented Dec 29, 2020

@JordanMartinez I do not feel discouraged from contributing (in fact, I went ahead and contributed purescript/purescript#3975 right away). Normally I hesitate to contribute because I have a very limited insight into what's going on, which problems are known and which are not, and which are already being worked on. One could argue that it's better to just contribute, and if I miss the mark - oh well, just close the PR. But that is also not ideal, because it wastes the maintainers' time on reviewing and closing spurious PRs. So I try to make sure what I contribute is actually useful.

In this particular case, I got an impression from your comment that this particular problem is already being worked on, and I'm just duplicating the effort and stealing valuable time. So I opted to stop the process ASAP to free up resources.

If this impression is mistaken, I would be very happy to reopen this PR and bring it to completion. As far as I understand, the plan should be:

  1. Remove generics-rep from libraries in purescript.dhall and purescript-contrib.dhall
  2. Fix up those libraries, where necessary (e.g. Data.Generics.Rep.Eq --> Data.Eq.Generic) via separate PRs to respective repos.
  3. Try to remove generics-rep dependency from all other libraries, see if any of them break as a result, then remove (by commenting out) those libraries from the package set.
  4. (later) Remove all code from the generics-rep repo and add a "migrated to prelude" message.

Let me know, I'd be happy to help out.

@JordanMartinez
Copy link
Contributor

Ok, good, I'm glad that wasn't how it came off. And yeah, I saw that you fixed the code in purescript. Thanks for that!

If this impression is mistaken, I would be very happy to reopen this PR and bring it to completion.

Actually, this was already done in #788. The CI works differently on this branch than master. Thanks though!

If you want to keep contributing, perhaps you can help with core library repo's indicated in Harry's list of repos. The list shows both issues that need to be resolved and/or PRs to finish/merge.

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.

4 participants