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

Filesystem - add argument values into InvariantViolationException message #205

Conversation

vaclavvanik
Copy link
Contributor

I mentioned in #204 that thrown InvariantViolationException messages should contain argument values. It's easier to debug and fix code.

I updated everything I have found. Unit tests just updated. No additional unit tests added.

Copy link
Owner

@azjezz azjezz left a comment

Choose a reason for hiding this comment

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

Thank you for working on this 👍

src/Psl/Filesystem/Internal/write_file.php Outdated Show resolved Hide resolved
@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes. labels Jun 3, 2021
@azjezz azjezz added this to the 1.8.0 milestone Jun 3, 2021
@azjezz
Copy link
Owner

azjezz commented Jun 3, 2021

P.S: you can regenerate the docs using php docs/documentor.php.

@coveralls
Copy link

coveralls commented Jun 3, 2021

Pull Request Test Coverage Report for Build 903482522

  • 24 of 24 (100.0%) changed or added relevant lines in 16 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.967%

Totals Coverage Status
Change from base Build 888968618: 0.0%
Covered Lines: 3064
Relevant Lines: 3065

💛 - Coveralls

@vaclavvanik
Copy link
Contributor Author

I see inconsistency in input argument $file vs $filename. Almost everywhere is $filename. Just in

  • append_file
  • Internal\write_file
  • read_file
  • write_file

is $file. I am not sure which is better. But I do not want to rename it in this PR.

@azjezz
Copy link
Owner

azjezz commented Jun 3, 2021

yea, i can see how that is confusing, we should rename $filename to $path or $node, as everywhere else it means "file or directory", while the functions you mentioned explicitly require a file, not a directory.

@vaclavvanik
Copy link
Contributor Author

Docs updated. I think I am done.

@azjezz
Copy link
Owner

azjezz commented Jun 3, 2021

make coding-standards-fix 😅

azjezz
azjezz previously approved these changes Jun 3, 2021
Copy link
Owner

@azjezz azjezz left a comment

Choose a reason for hiding this comment

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

Thank you @vaclavvanik!

@azjezz
Copy link
Owner

azjezz commented Jun 3, 2021

ops, doc needs another update ( you can run checks locally using make check ( now i see that it doesn't check documentation, probably need to update that later on )

@vaclavvanik
Copy link
Contributor Author

I have updated docs. Now should be everything ok.

@azjezz azjezz merged commit 3d9bb5b into azjezz:1.8.x Jun 3, 2021
@azjezz azjezz added Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness and removed Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. labels Jun 3, 2021
@vaclavvanik vaclavvanik deleted the filesystem-invariant-violation-exception-message-file branch June 3, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filesystem - read_file - InvariantViolationException message should contains filename
3 participants