Skip to content

adds support for jdbcUrls in the format protocol://user:password@host - #14

Closed
alekcz wants to merge 6 commits into
replikativ:masterfrom
alekcz:master
Closed

adds support for jdbcUrls in the format protocol://user:password@host#14
alekcz wants to merge 6 commits into
replikativ:masterfrom
alekcz:master

Conversation

@alekcz

@alekcz alekcz commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

Most managed dbs use the format protocol://user:password@host/db. This isn't the preferred approach in next.jdbc. This PR improves the postgres support.

Also:

  • Fixes linter errors
  • Fixes reflection in release
  • Adds tests for jdbcUrl
  • Updates next.jdbc version

@alekcz
alekcz requested review from TimoKramer and jsmassa April 23, 2023 17:26
@jsmassa

jsmassa commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator

When I am trying to run the tests locally, I am getting

Error building classpath. Failed to read artifact descriptor for com.github.seancorfield:next.jdbc:jar:1.3.874

Did it work for you?

Otherwise it might be my local setup.

@jsmassa

jsmassa commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator

Otherwise, please note that we usually work with squashed commits for PRs, so cleanups and functional changes should be in separate PRs. For now, it's ok in this repo, I'd say, as long as all the single commits will end up in master and the commits are clean.

As far as I can see, following commits mix cleanups and functional changes

So it would be good to
a) either clean those commits (i.e. separate the commit into the functional changes and cleanups) and do a "rebase merge" or
b) redo the PR with only the functional changes, in this case you can do a "squash merge".

(FYI, there is already PR #12 for cleanups, partially the same that you did. I am not opposed to closing that if the cleanups are merged with a different PR, but it could also just get updated.)

I am not opposed to doing cleanups in the files you are touching anyway and I can totally understand that you want to fix things immediately that are not perfect. I do feel that drive as well and I don't always succeed in pacing myself either.

Reviews become quite difficult though when most of the changes are cleanups of unrelated code. Also, more files changed usually means it needs more time to review, so it might take longer until someone takes the time to review a PR.

The other reason to separate those things is a meaningful file history. A commit called "cleanup" will less likely be the reason for a problem than some functional change will be, so it makes sense to look at them last when tracking down the appearance of a bug.

@alekcz

alekcz commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator Author

@jsmassa that's one on clojars https://clojars.org/com.github.seancorfield/next.jdbc.
Lemme close this one and do it from scratch without the clean up stuff

@alekcz alekcz closed this Apr 24, 2023
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.

2 participants