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

Added seed data file and importer #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

theghall
Copy link
Collaborator

@theghall theghall commented May 13, 2023

The data file does not have all the fields that are in units table. And some units were missing the DC, so I just put in -1. I didn't seed all the tables, but thought this is a good place to commit and PR, otherwise this branch may sit locally for awhile.

Copy link
Owner

@AphonicChaos AphonicChaos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I like the spirit of the PR, but think there are some improvements that could be made. I've commented with specific suggestions to hopefully make those clear.

I also think alembic or FastApi might have a built-in concept for seeding databases (Flask and Django both have the concept of database fixtures which does this". I'll look around real quick and see if I can find a suitable framework and report back with what I find.

connection.close()

def setup_tables():
global unit_faction_links_table, unit_factions_table, units_table
Copy link
Owner

Choose a reason for hiding this comment

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

Please avoid using globals. It's a code smell and should be unnecessary.

db_metadata = db.MetaData()
connection.close()

def setup_tables():
Copy link
Owner

Choose a reason for hiding this comment

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

You also shouldn't need a setup_tables function, or even. You can set up tables already by using python -m alembic upgrade head

db.Column("health", db.Integer),
db.Column("deployment_cost", db.Integer))

def reset_tables():
Copy link
Owner

Choose a reason for hiding this comment

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

This can be accomplished with an alembic downgrade. Specifically, python -m alembic downgrade base ought to do what you're doing here.

"health"
]

value_list = {}
Copy link
Owner

Choose a reason for hiding this comment

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

A dictionary comprehension would make this cleaner. Also, because you're creating a dictionary, I wouldn't name it _list:

value_dict = {
    value_dict[unit_column_map[column]]: unit_entry[column] 
    for column in unit_entry if column in unit_column map
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I'm doing here is building a one row value list for the insert in store_unit_data. I did this because at the time it seemed easier to map the fields from the file to the db fields into a value list than doing #insert(name=unit_entry["name"], etc.). but see my other comment below.

@@ -0,0 +1,643 @@
[
{
"factions": ["AC", "EMP", "MW", "ISA"],
Copy link
Owner

Choose a reason for hiding this comment

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

Could we spell these out? "Aeternus Continuum", "Empyrian", etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment below.

[
{
"factions": ["AC", "EMP", "MW", "ISA"],
"type": "SOLO",
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer title-case for the types, and no underscores for qualities ("Wild Card"), please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment below.

@AphonicChaos
Copy link
Owner

In regards to my comment here, I did a brief look around and it looks for our specific library selection (SQLModel + FastAPI), there aren't really pre-built options out there. For the library SQLModel is built on top of, SQLAlchemy, there aren't really any maintained options.

I think I'd be ok with a utility seed script if the json being loaded was a bit more mapped to the tables, but I also understand what you have here took time and effort. If I were to consume json, I'd probably do so using a format similar to this:

[
    {
        "model": "Unit",
        "values": [
            {
                "id": 1,
                "name": "Scourge",
                ...
            }
        ]
    }
]

I can easily transform to something like the above when I get back to coding though using jq to automate the task, so I'm happy to merge what you have instead for now, assuming you address my comments about some of what you submitted already being available, remove the globals, etc.

@theghall
Copy link
Collaborator Author

theghall commented May 23, 2023

In regards to my comment here, I did a brief look around and it looks for our specific library selection (SQLModel + FastAPI), there aren't really pre-built options out there. For the library SQLModel is built on top of, SQLAlchemy, there aren't really any maintained options.

I think I'd be ok with a utility seed script if the json being loaded was a bit more mapped to the tables, but I also understand what you have here took time and effort. If I were to consume json, I'd probably do so using a format similar to this:

[
    {
        "model": "Unit",
        "values": [
            {
                "id": 1,
                "name": "Scourge",
                ...
            }
        ]
    }
]

I can easily transform to something like the above when I get back to coding though using jq to automate the task, so I'm happy to merge what you have instead for now, assuming you address my comments about some of what you submitted already being available, remove the globals, etc.

Actually, this file was given to me. I believe it is from BattleScribe. I did find the XML file I was looking for, but it was woefully out of date with the releases. I did spend a fair amount of time getting rid of the trailing commas; that is getting the regex correct. As for the other file comments, I was doing this after the kids went to bed. It now dawns on me I could do a search and replace and match the data file fields to the db fields, plus spell out the factions. As for the missing fields in the data file I was envisaging someone who wanted to contribute, but wasn't a coder who could fill in missing fields and also make corrections. I won't get to it tonight. I've been up since 2:30 wrangling my special needs kids and then trying to be productive at work, which surprisingly I was. I'll fix the file and make the other changes sometime this week and then ping you for another review.

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