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

RFC 65 - change type of Env.root to be AmbientAuth #3558

Closed
wants to merge 2 commits into from
Closed

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented May 12, 2020

Closes #3557

@mfelsche mfelsche added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label May 12, 2020
@SeanTAllen
Copy link
Member

@mfelsche can you write up release notes and how to change on the next release notes?

@SeanTAllen SeanTAllen mentioned this pull request May 12, 2020
@SeanTAllen
Copy link
Member

SeanTAllen commented May 13, 2020

@mfelsche I opened a PR for the update to the tutorial:

ponylang/pony-tutorial#433

Also needing updates ready before this is merged:

  • ponyup
  • corral
  • pony-patterns (2 patterns- one Supply Chain requires extensive work)
  • changelog-tool
  • http-server
  • http
  • glob
  • crypto (no PR needed)
  • net_ssl
  • peg
  • appdirs (no PR needed)
  • reactive_streams (no PR needed)
  • regex (no PR needed)
  • semver
  • release notes

@mfelsche
Copy link
Contributor Author

I can do the release notes.
@SeanTAllen where should i put them?

i can also do the rest of the listed pony libraries

@SeanTAllen
Copy link
Member

@mfelsche on the release issue.

@SeanTAllen
Copy link
Member

@mfelsche please update the checklist of things to do before this can be merged as you do them. I checked off net_ssl

@jemc
Copy link
Member

jemc commented May 26, 2020

We discussed this in today's sync call.

Sylvan shared an example of an alternative POC approach that he came up with that would allow env.root to be something besides just AmbientAuth | None: https://playground.ponylang.io/?gist=f0dca3abd61d28d2c9ad806393d3c025

If someone is able to write an RFC for that approach, and we decide we like it, we may want to hold off on merging this and consider not breaking everyone's env.root-using code twice (once for this RFC, and then again for Sylvan's).

That said, this RFC has actually been accepted and the other one hasn't even been written, so I don't want to actually block this PR if @mfelsche wants to move forward with it.

@SeanTAllen
Copy link
Member

We discussed again, we would prefer the Sylvan approach as noted in the playground example in the link above. We don't plan on merging this change, however, we need an RFC to be written for the "Sylvan change" to move forward with it.

@SeanTAllen
Copy link
Member

We've opened an RFC issue for the "Sylvan approach": ponylang/rfcs#177

@SeanTAllen
Copy link
Member

@mfelsche comments?

@mfelsche
Copy link
Contributor Author

The AuthSet approach is totally fine. Lets rather have this as a basis for solving issues around env.
We can close this one.

@SeanTAllen SeanTAllen closed this Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Change type of Env.root to AmbientAuth
3 participants