-
Notifications
You must be signed in to change notification settings - Fork 668
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
disussion#7862: adds first test case #7890
base: master
Are you sure you want to change the base?
disussion#7862: adds first test case #7890
Conversation
tests/ReportOutputTest.php
Outdated
$file_contents = <<<'EOT' | ||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the master branch requires PHP 7.4, I'd prefer the indented heredoc syntax. Not sure if @orklah has any thoughts on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never use heredoc so I don't really know, but yeah, if we can indent that, it's probably for the best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, just indent everything including EOT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I elaborate first crappy crappy version!
The idea is:
- Detect from the "issue->message" two arrays, requested and provided
to perform this operation I create a class "PrettyDetectArray->detect"
This is a complex operation because the message is a "random" payload!
The next idea might add some sort of meta-data, to the point where detecting the issue.
In this case, the process is simple: format and compare... (next in work...)
-
Format one and second array
to perform this operation I create a class "PrettyFormat->format" -
Compare one and second array
to perform this operation I create a class "PrettyCompare->compare"
I think to improve the concrete code, to make it more clean and readable
push coming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detect from the "issue->message" two arrays, requested and provided to perform this operation I create a class "PrettyDetectArray->detect"
This seems unnecessarily complicated and will probably cause issues if I do something like class Myarray
Myarray{key: 'value'}
. I think it would probably be easier and better to do something like add a $types
list to issues and have the message be a format string. You could even be more generic and support multiple rendering categories in the future:
new SomeIssue('The type "{provided}" doesn't match the expected "{expected}"', ["provided" => $provided_type, "expected" => $expected_type], $suppressed_issues);
On the other hand having to track the type in the issue requires more refactoring, so idk. If we do decide to do a refactor requiring a change to issue messages, we might as well try to be consistent about the order of "Foo provided, expected Bar" vs "Expected Foo, found Bar" everywhere.
Really nice to get this improvement though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it a bit more, I think if we went the route of having the issues track the actual Union
types, we'd probably want to make the message generic like 'The provided type doesn't match the expected type'
, and then when it's rendered that can be converted to either 'The provided...type. Provided: array{...}; Expected: array{...}'
or the new pretty format based on config and output type.
If you've already gotten the type detection mostly working though, that might be easier to go with for now until things get closer to finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice !
Really nice to get this improvement though!
I'm ready to go in this direction for me is the best!
I could create a intermediate task/issue to do this work, then complete this ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the second sentence?
Thinking about it a bit more, I think if we went the route of having the issues track the actual Union types, ...
like structure with the "tokens" ?
10e4fba
to
d398f05
Compare
Sorry for not triggering CI earlier, I have some backlog on replying to issues and PR. Don't hesitate to ping me if needed ;) |
I will try some spyke, for the next step |
…-7862' into pr-prettry-print-from-discussion-7862 # Conflicts: # src/Psalm/Report/ConsoleReport.php # tests/ReportOutputTest.php
9fb9fd2
to
62b57b6
Compare
I will try to simulate windows env with linux ... |
This reverts commit 935806f.
|
small recap:
|
Good news! through this #8034 I fixed the code. Example of the output:
now I work to fix the "CI". I think that soon I will open PR for the review. |
If you want, find a little something to PR so I can merge it. You won't be bothered by manual approval on our end anymore after that ;) |
☝️ There are plenty of issues marked as easy or good first issue, I generally don't bother with the easy ones that I don't personally care about so that there are good ways for new contributors to get started. |
OK, so I start with an issue with this label? |
#8062 should be fairly easy for example |
This PR poin to implements the idea from this
#7862