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

Facturx 2.3 #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

guillaume-sainthillier
Copy link
Contributor

Hello there, i've added more fields from official specification files with tests

Copy link
Member

@BolZer BolZer left a comment

Choose a reason for hiding this comment

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

Hi @guillaume-sainthillier. Sorry for the late response and thank your for your contribution.

The PR is quite large and i would like you to ask, that you shrink it to just adding the examples, and adding the fields which are missing.

There are some refactors that i would like to discuss. Therefore, i opened the discussion board in this repository.

Comment on lines +36 to +37
/** @return iterable<int, array<SplFileInfo>> */
public function dataProvider(): iterable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @return iterable<int, array<SplFileInfo>> */
public function dataProvider(): iterable
/** @return iterable<string, array<SplFileInfo>> */
public function dataProvider(): iterable

@@ -44,7 +44,7 @@ public function dataProvider(): array

$buffer = [];
foreach ($finder as $file) {
$buffer[$file->getFilename()] = [$file];
yield [$file];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield [$file];
yield $file->getFilename() => [$file];

Your change to generator is fine. But i would like to have the filename as dataset name. If the test fails, the offending example is easily found.

@guillaume-sainthillier
Copy link
Contributor Author

hello @BolZer, thanks for your update!

What would you like me to remove in this PR? I can surely remove the yield from phpunit tests provider but mainly other changes are related to adding new fields and making tests pass.

Are you talking about changes related to uses of AccessorOrder?

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

Successfully merging this pull request may close these issues.

2 participants