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

Database version Rename #28

Merged
merged 4 commits into from
Nov 16, 2018
Merged

Database version Rename #28

merged 4 commits into from
Nov 16, 2018

Conversation

cblgh
Copy link
Member

@cblgh cblgh commented Nov 14, 2018

i renamed protocol version to database version and used an int to represent it instead as it doesn't make sense to have minor & patch on something like this.

lmk what else we can do here before we merge into master

this will be required for cabal-cli's multi cabal pr

@okdistribute
Copy link
Member

What other modules break by changing the external api for protocolVersion to databaseVersion? This would probably be a major bump of cabal-core.

Using a separate version number than the core library that's published on npm might cause confusion later in time. With a small distributed set of contributors, reducing the number of variables at play can help with groking potential bugs or mismatches in versions. What if we used the version from package.json?

@cblgh
Copy link
Member Author

cblgh commented Nov 14, 2018

@Karissa nothing will break as nobody is relying on protocolVersion yet, it's just the multi-cabal pr right now. which is why i felt okay with doing a breaking rename like this

(also there's a major bump in there too) i'm mixin' my semvers up because i'm too fast 🔥

see the discussion here #25

package.json Outdated Show resolved Hide resolved
@@ -66,9 +66,9 @@ function Cabal (storage, key, opts) {
}

inherits(Cabal, events.EventEmitter)
Cabal.prototype.getProtocolVersion = function (cb) {
Cabal.prototype.getDatabaseVersion = function (cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the code that changes the ~/.cabal/archives dir name already in master, or can that get into this PR too?

Copy link
Member Author

@cblgh cblgh Nov 14, 2018

Choose a reason for hiding this comment

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

@noffle it's not in master, but in the multi-cabal pr here cabal-club/cabal-cli#89

(i'm not sure how i would bring it into the cabal-core pr)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of course!

My thinking was "gosh it'd be nice if we could get in the db-dir changes so we can test that separately from the multi-cabal work".

@cblgh cblgh merged commit 12bafdd into master Nov 16, 2018
@cblgh cblgh deleted the database-version-rename branch April 18, 2020 11:26
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.

3 participants