Skip to content

Allow create database in python#23

Open
flywire wants to merge 16 commits intopfalcon:masterfrom
flywire:patch-1
Open

Allow create database in python#23
flywire wants to merge 16 commits intopfalcon:masterfrom
flywire:patch-1

Conversation

@flywire
Copy link
Contributor

@flywire flywire commented Feb 24, 2025

close #22

@flywire flywire mentioned this pull request Feb 24, 2025
@pfalcon
Copy link
Owner

pfalcon commented Feb 25, 2025

This PR is hosed by now and should be reworked.

It should be an option to create a database, default behavior of the application shouldn't change.

@flywire
Copy link
Contributor Author

flywire commented Feb 25, 2025

Agree. There are some conflicts with #24 so let's see your thoughts on that first.

Otherwise, prune it after e1b2948 (possibly)

@flywire
Copy link
Contributor Author

flywire commented Feb 25, 2025

It should be an option to create a database, default behaviour of the application shouldn't change.

Only two commits are relevant:

  1. df5e2ec - Allow create database in python
  2. e1b2948 - Enable create database in python

I understand you are supportive of the first commit but not the second. How would you enable the first?

import logging
import os
import sqlite3
from pathlib import Path
Copy link
Owner

Choose a reason for hiding this comment

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

Unused import, and please don't add any dependencies on pathlib (it doesn't do anything which Python couldn't do 20 years ago).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you say, code was in fork I adapted.

@pfalcon
Copy link
Owner

pfalcon commented Feb 25, 2025

What I meant is that there should be option like --createdb which would trigger execution of schema scripts. Implementation wise, it then probably should be a separate function like init_schema() in dbhelper.

I'm also not a fan of all the novelties like pathlib and f-strings. Whereas I'm a fan of minimalism, so prefer to rely on decades-known features vs recent novelties (where ones being replaced by others in rat-race).

@flywire
Copy link
Contributor Author

flywire commented Mar 3, 2025

I'm also not a fan of all the novelties like pathlib and f-strings.

Do you consider this PR worth accepting if I rework based on the two commits and a --createdb option leaving you to tweak to your preferred coding style?

@pfalcon
Copy link
Owner

pfalcon commented Mar 9, 2025

@flywire

Do you consider this PR worth accepting

I remember it was one of your older concern that Makefile is used to create a database. It's not a concern to me, but I'm happy to listen to contributors who are willing to do legwork to change that (I myself wouldn't be willing to do legwork, because it already works and doesn't change much IMHO).

if I rework based on the two commits and a --createdb option leaving you to tweak to your preferred coding style?

Sorry, I unlikely would be able to tweak it any time soon, because I have hundreds of my own patches to tweak to make them suitable for upstreaming (e.g., for PP itself, but not only). I'm happy to provide review comments and suggestions, that's how open source contribution process usually works.

Thanks.

@flywire
Copy link
Contributor Author

flywire commented Jul 4, 2025

Only two commits are relevant:

  1. df5e2ec - Allow create database in python
  2. e1b2948 - Enable create database in python

I understand you are supportive of the first commit but not the second. How would you enable the first?

Your response is still not clear to me. Would you be supportive of the first (Allow create database in python) without the second (Enable create database in python)? It would be trivial for technical users to flip the boolean without stuffing around with options etc.

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.

Create Database in Python

3 participants