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

First batch of PRs #1

Closed
bstaletic opened this issue Apr 9, 2019 · 3 comments
Closed

First batch of PRs #1

bstaletic opened this issue Apr 9, 2019 · 3 comments

Comments

@bstaletic
Copy link
Collaborator

I don't feel comfortable pushing to master, so I'll post a list of what I've gone through so far.

@henryiii
Copy link
Collaborator

henryiii commented Apr 9, 2019

Can't you make this at least a PR? There's nothing in master. And having to copy/paste your comments in kind of defeats the purpose.

A few notes:

  • For 1043: tests were added, this fixes a bug that previously was not part of the tests.
  • For 1101, I think with some time and maybe some help I could get the declaration right. I've marked it WIP.
  • For 1122: I think it needs to be based on whatever we do with enums. Having a string constructor for enums enables a user to turn on implicit conversion from strings, which would be very useful.
  • For 1131: Jakob didn't understand the PR in the first comment, once he did (in the second comment) he didn't have any performance concerns. It was approved but never merged.

@bstaletic
Copy link
Collaborator Author

Alright, I'll make a PR and close this in a moment.

@bstaletic
Copy link
Collaborator Author

Check #2 out.

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