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

Create location directory recursively #6

Merged
merged 2 commits into from
Mar 6, 2022
Merged

Create location directory recursively #6

merged 2 commits into from
Mar 6, 2022

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Mar 5, 2022

Just more convenient. Any reason we haven't added this before?

@juliangruber
Copy link
Member

Maybe this was discussed 🤔

I think I'm against this as it breaks with expectations. If you want to create a file somewhere, if the parent directory doesn't exist you receive an error. I can't think about a particular example but I think APIs should follow the law of least surprise.

If anything I think we could add this as an option, but then people could just use their own recursive directory making tooling instead.

Copy link
Member

@ralphtheninja ralphtheninja left a comment

Choose a reason for hiding this comment

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

Nitpick.

README.md Outdated Show resolved Hide resolved
@vweevers
Copy link
Member Author

vweevers commented Mar 5, 2022

@juliangruber My motivations:

  • It already works like this on Windows and this platform difference has bitten me several times over. E.g. working on Windows then deploying to Linux. It's easier atm to align the behavior in JS than it is to align in C++. Although I believe it's a behavior of our port - that newer versions of LevelDB don't have. But I don't want to touch those parts until after the abstract-level effort.
  • Seen in the wild that people wrap db.open() to achieve this, not particularly pretty.
  • Convenient for tests. I frequently do e.g. new Level('./db/ + Date.now()) so that test databases are created in a single directory that is close by and can be removed in one go, and ./db does not have to exist beforehand.
  • I mostly use sublevels now but in the past I've done posts = level('./db/posts') to create isolated databases (not for tests).

As for breaking expectations, the Windows behavior and current lack of documentation shows that there are either no expectations, or conflicting expectations.

Most importantly, if I'm doing new Level('foo/bar/baz') then I have a reason for doing so and I want that to "just work". The database abstracts away dealing with fs.

@vweevers
Copy link
Member Author

vweevers commented Mar 6, 2022

@juliangruber I'm merging, because this aligns behavior between platforms. If you feel strongly about it we can revert later, at which point we'll need to fix the Windows C++ (perhaps by upgrading LevelDB) which would be semver-major anyway. I do realize that user expectations may have changed by then, and that not waiting for your response is therefor not entirely fair. If need be we'll doc-deprecate the behavior.

@vweevers vweevers merged commit 1ba0b69 into main Mar 6, 2022
@vweevers vweevers deleted the mkdirp branch March 6, 2022 14:29
@juliangruber
Copy link
Member

Ok, if we release this it'll be a major change on mac/linux

@vweevers
Copy link
Member Author

vweevers commented Mar 7, 2022

Do you mean semver-major? Because I don't see why it should be. Which goes back to our conflicting expectations, but even if you expect an error, what code would actually break by this change?

Or do you mean that it warrants documentation in a special place? Like an upgrade guide.

@juliangruber
Copy link
Member

I mean semver major, yeah, since code can rely on it not recursively creating directories. Like, consumer code could try/catch the db creation and perform some special bootstrap code for example. It's tough to know, and maybe this isn't breaking anyone's application in reality, but strictly speaking it is a breaking change I think.

@juliangruber
Copy link
Member

I just found the issue where this was discussed before, my memory served me right :D

Level/levelup#135

@vweevers
Copy link
Member Author

vweevers commented Mar 8, 2022

OK. Considering where we are - classic-level which has not seen any downloads yet - I'll add this change to the upgrade guide of Level/level#215.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor New features that are backward compatible
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants