Use dict/keyset/vec - Githubissues

hhvm / hack-codegen

Library to programatically generate Hack code and write it to signed files

https://hhvm.github.io/hack-codegen/

MIT License

341 stars 93 forks source link

Use dict/keyset/vec #50

Closed Neitsch closed 7 years ago

Neitsch commented 7 years ago

Added hhvm/hsl
Changed any occurence of ConstVector/ImmuVector/Vector to vec
Changed any occurence of ImmuMap/Map to dict
Changed any occurence of ImmuSet/Set to keyset
Used HSL wherever possible

Problems:

Something with using namespaces was going wrong for me, so I switched to using the fully qualified name

Notes:

Using dict/keyset/vec instead of the legacy types will be a breaking change. I'm happy to adapt my PR to introduce separate methods that create the new collection types.
Sorry that the PR is so big :/

facebook-github-bot commented 7 years ago

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

fredemmott commented 7 years ago

Thanks; sorry, but I won't be merging this while the HSL is an experimental preview - though I definitely want this in the next few months.

I also won't be merging this with fully qualified names - were you using use namespace HH\Lib\.... or use HH\Lib\... ? If you were using use namespace, can you share what problems you were getting?

fredemmott commented 7 years ago

Also, the bot is correct about needing to email cla@fb.com: you were previously covered under another agreement, and our self-service tooling doesn't support replacing that with an individual agreement.

fredemmott commented 7 years ago

Using dict/keyset/vec instead of the legacy types will be a breaking change. I'm happy to adapt my PR to introduce separate methods that create the new collection types.

breaking change is fine; I'm likely to create a 3.x branch that requires 3.21+, potentially with other changes (I'm hoping to re-unify the internal and public versions)
support for generating code with the existing types must be retained - similar to how creating PHP arrays is currently supported
- functions with Vector/Map/Set in the name shouldn't be related to vec/dict/keyset, that'll be confusing

Neitsch commented 7 years ago

CLA: In progress :)

Namespaces: I'll set up a separate branch showing how I set it up. Maybe you can help me out fix the problem(s). Tbh, never had to deal with the whole autoloading stuff before.

This PR: I'm going to split this PR into two. This one will contain the internal switch to vec/dict/keyset. I'll make a separate PR to add vec/dict/keyset for codegen and address the comments.

facebook-github-bot commented 7 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

fredemmott commented 7 years ago

Do you still want this reviewed, or just split PRs?

fredemmott commented 7 years ago

That said, please avoid Str\split and Str\join for now (keep using explode/implode), given https://github.com/hhvm/hsl/issues/3 and the fact that the typechecker won't be able to point out the problems when Str\split changes

Neitsch commented 7 years ago

I'll just made a separate PR. First tried to refactor the other changes out, but turned out to be a mess.