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.

Sign up for GitHub

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

Jump to bottom

[Type] replace Type\arr by Type\{mutable_}map and Type\{mutable_}vector #101

Merged
merged 1 commit into from
Nov 8, 2020
Merged

[Type] replace Type\arr by Type\{mutable_}map and Type\{mutable_}vector #101

merged 1 commit into from
Nov 8, 2020

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Nov 7, 2020 •
edited
Loading

closes #100

@azjezz azjezz added Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes. labels Nov 7, 2020
@azjezz azjezz added this to the 0.1.0 milestone Nov 7, 2020
@azjezz azjezz self-assigned this Nov 7, 2020
@coveralls
Copy link

coveralls commented Nov 7, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 737

  • 113 of 113 (100.0%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 718: 0.0%
Covered Lines: 1948
Relevant Lines: 1948
💛 - Coveralls

@azjezz azjezz force-pushed the feature/type-map-vector branch 2 times, most recently from 181b65f to 39f9782 Compare November 7, 2020 22:20
@veewee
Copy link
Collaborator

veewee commented Nov 8, 2020

Was thinking : does it make sense to remove the array type altogether? In some situations in which you dont have that much control over de source data, you might be ok with having a very loose regular mixed array.

@azjezz azjezz force-pushed the feature/type-map-vector branch 2 times, most recently from bf178d7 to 31a7eb3 Compare November 8, 2020 13:44
@azjezz azjezz marked this pull request as ready for review November 8, 2020 13:45
@azjezz azjezz requested a review from veewee November 8, 2020 13:45
@azjezz azjezz force-pushed the feature/type-map-vector branch 2 times, most recently from 0c6aef0 to 9725ce5 Compare November 8, 2020 13:50
azjezz
azjezz commented Nov 8, 2020
View reviewed changes
tests/Psl/Json/TypedTest.php Outdated
Comment on lines 38 to 44
/** @var MapInterface $actual */
$actual = Json\typed(
'["php", "std", "stdlib", "utility", "psl"]',
Type\vector(Type\string())
);

static::assertInstanceOf(VectorInterface::class, $actual);
static::assertSame(['php', 'std', 'stdlib', 'utility', 'psl'], $actual->toArray());
Copy link
Owner Author

Choose a reason for hiding this comment

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

cc @veewee 😉

@azjezz azjezz added Status: Review Needed The issue has a PR attached to it which needs to be reviewed. and removed Status: In Progress This issue is being worked on, and has someone assigned. labels Nov 8, 2020
@azjezz azjezz changed the title [Type] replace Type\arr by Type\map and Type\vector [Type] replace Type\arr by Type\{mutable_}map and Type\{mutable_}vector Nov 8, 2020
@azjezz
Copy link
Owner Author

azjezz commented Nov 8, 2020

Was thinking : does it make sense to remove the array type altogether? In some situations in which you dont have that much control over de source data, you might be ok with having a very loose regular mixed array.

that is just Type\map(Type\array_key(), Type\mixed())->coerce($var)->toArray()

@azjezz
Copy link
Owner Author

azjezz commented Nov 8, 2020

but i think we can add Type\untyped_array if you are up for a PR 😄

@veewee
Copy link
Collaborator

veewee commented Nov 8, 2020

Sure, Will see what I can do. Hopefully at the end of the week :)

veewee
veewee approved these changes Nov 8, 2020
View reviewed changes
Copy link
Collaborator

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Looks good! Added a small remark / question.
Gonna try to open up a PR for maing the type layer pure as well. Still got some issues there.

src/Psl/Type/Internal/MutableMapType.php
}

/** @psalm-var Iter\Iterator<Tk, Tv> $iterator */
$iterator = Iter\from_entries($entries);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense not to use entries but build the map up from the foreach directly? This would allow for pure / immutable type classes (since from_entries($iterable) is not pure)

(same goes for the other usages of entries)

Copy link
Owner Author

Choose a reason for hiding this comment

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

it woudln't make any differencce here, since coerce accepts any iterable, and does a foreach on it, meaning it can never be pure. as that iterable we are doing foreach on isn't really immutable.

@azjezz
Copy link
Owner Author

azjezz commented Nov 8, 2020

Gonna try to open up a PR for maing the type layer pure as well. Still got some issues there.

not sure if that's possible, but we will see, i'm merging this as-is for now.

@azjezz azjezz merged commit 2bef63d into 0.1.x Nov 8, 2020
@azjezz azjezz deleted the feature/type-map-vector branch November 8, 2020 15:28
@veewee
Copy link
Collaborator

veewee commented Dec 4, 2020

Gonna try to open up a PR for maing the type layer pure as well. Still got some issues there.

not sure if that's possible, but we will see, i'm merging this as-is for now.

Put some more time in it and it turns out it is not possible ...
As long as we cannot declare immutable iterables, this means it won't be possible unless we change everything to a regular array, which doesn't seem optimal to me in this case.

@veewee veewee mentioned this pull request Dec 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veewee veewee veewee approved these changes

Assignees

@azjezz azjezz

Labels
Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.

[Type] Missing way of declaring map/vector types
3 participants
@azjezz @coveralls @veewee