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

Report yaml syntax errors in more detail #662

Merged
merged 6 commits into from
Aug 3, 2021
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 3, 2021

fixes #661

more details in yaml syntax errors

grafik

$exception->getMessage()
));
}

public static function fromFilename(string $filename): self
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method then still in use?

Copy link
Contributor Author

@staabm staabm Aug 3, 2021

Choose a reason for hiding this comment

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

it seems it is only used from within a test. not sure it make sense to delete it though for BC reasons. marking as deprecated could be an option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no BC consideration if there is no public user-facing API, based on the usage of this tool. So from that perspective, it should be fine to delete it. The fact that it is used only for testing is problematic and IMHO symptom of bad design. If you are able to rewrite the test to not use it, it is IMHO safe to delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed the method and adjusted the test-case for it to actually test the newly introduced method

Copy link
Collaborator

@dbrumann dbrumann left a comment

Choose a reason for hiding this comment

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

Thanks, @staabm.

@dbrumann dbrumann merged commit 3b62b7d into qossmic:main Aug 3, 2021
@staabm staabm deleted the yaml-syntax branch August 3, 2021 17:41
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.

print yaml error
5 participants