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

Imrovements on file creation during write modes #401

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

veewee
Copy link
Collaborator

@veewee veewee commented Feb 27, 2023

There are some inconsistencies between ReadWriteHandle and WriteHandle.
The first one tries to create the file (and directories), the other does not.
The logic inside the handles are not in line with the docblocks above WriteMode: If all write-modes are trying to create the file, it doesn't make sense to disallow creation during e.g. APPEND and TRUNCATE.

This PR is a little draft to discuss a better approach that is consistent in between both handles and in line with the dockblocks of the write mode.

@veewee veewee marked this pull request as draft February 27, 2023 19:19
@veewee veewee changed the base branch from next to 2.4.x February 27, 2023 19:20
@coveralls
Copy link

coveralls commented Feb 27, 2023

Pull Request Test Coverage Report for Build 4293361536

  • 23 of 23 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 99.15%

Totals Coverage Status
Change from base Build 4015307350: 0.03%
Covered Lines: 3968
Relevant Lines: 4002

💛 - Coveralls

@veewee veewee force-pushed the create-file-on-write branch 2 times, most recently from 2103315 to 0ca1ceb Compare February 27, 2023 19:26
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.

overall looks good, I'll have another look tomorrow when I'm a bit more caffeinated 😁

tests/unit/File/ReadWriteHandleTest.php Outdated Show resolved Hide resolved
@veewee veewee marked this pull request as ready for review February 28, 2023 13:21
@veewee veewee added 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 Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. labels Feb 28, 2023
@veewee veewee requested a review from azjezz February 28, 2023 13:22
@veewee veewee changed the base branch from 2.4.x to next March 8, 2023 09:40
@veewee veewee merged commit 0b1eac3 into azjezz:next Mar 17, 2023
renovate bot referenced this pull request in ben-challis/sql-migrations Mar 18, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [amphp/file](https://togithub.com/amphp/file) | require | patch |
`3.0.0-beta.6` -> `3.0.0` |
| [azjezz/psl](https://togithub.com/azjezz/psl) | require | minor |
`2.4.1` -> `2.5.0` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>amphp/file</summary>

### [`v3.0.0`](https://togithub.com/amphp/file/releases/tag/v3.0.0):
3.0.0

[Compare
Source](https://togithub.com/amphp/file/compare/v3.0.0-beta.6...v3.0.0)

Stable release compatible with AMPHP v3 and fibers! 🎉

As with other libraries compatible with AMPHP v3, most cases of
parameters or returns of `Promise<ResolutionType>` have been replaced
with `ResolutionType`.

-   Renamed `BlockingDriver` to `BlockingFilesystemDriver`
-   Renamed `EioDriver` to `EioFilesystemDriver`
-   Renamed `ParallelDriver` to `ParallelFilesystemDriver`
-   Renamed `StatusCachingDriver` to `StatusCachingFilesystemDriver`
-   Renamed `UvDriver` to `UvFilesystemDriver`
-   Renamed `Amp\File\Sync\AsyncFileMutex` to `Amp\File\FileMutex`
-   Added `?Cancellation` as first parameter of `File::read()`
-   Added `File::isSeekable()`
-   Removed `File::SEEK_SET`, `File::SEEK_CUR`, and `File::SEEK_END`
-   Added `Amp\File\Whence` for seeking instead

</details>

<details>
<summary>azjezz/psl</summary>

### [`v2.5.0`](https://togithub.com/azjezz/psl/releases/tag/2.5.0):
Lenalee - 2.5.0

[Compare Source](https://togithub.com/azjezz/psl/compare/2.4.1...2.5.0)

#### What's Changed

##### Features

- Introduce Result\try_catch shortcut by
[@&#8203;veewee](https://togithub.com/veewee) in
[https://github.com/azjezz/psl/pull/403](https://togithub.com/azjezz/psl/pull/403)

##### Fixes

- Imrovements on file creation during write modes by
[@&#8203;veewee](https://togithub.com/veewee) in
[https://github.com/azjezz/psl/pull/401](https://togithub.com/azjezz/psl/pull/401)

**Full Changelog**: azjezz/psl@2.4.1...2.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/ben-challis/sql-migrations).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMC4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTAuMiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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 Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants