2024 _47b; Add db models #4

kami-nashi · 2024-11-22T02:47:09Z

With the basic changes, I'm able to import ice_time.py into a file and build a query that returns expected data.

from models import ice_time as time

ashley = time(1).ice_time_in_minutes()
inara = time(2).ice_time_in_minutes()

print(ashley, inara)

python scratch.py -> 45 285

If this file structure and class structure is sensible, the remaining queries will be added from legacy skatetrax to this commit.

…a config to be pulled from postgress db

…ction for adding more coaches

…th in the future. updating any import references. lastly, adding new models to setup_db.py

kami-nashi · 2024-11-28T01:28:41Z

This should now satisfy the items in #2.

PaulWay · 2024-11-28T02:35:42Z

models/__init__.py

+
+    class add():
+
+        def boot(boots):


Class methods will have the first argument as self. Is this what you want?

I have to admit, I'm not sure about this, and in a few ways. Ive been able to use this code to insert rows as expected..... so self being the first arg doesnt seem to affect anything... How ever, I'm not sure how/why that makes an impact here because Im lacking the understanding behind why this is the behavior.

Ultimately, i want to structure things in a way that i can import models, which may actually be the wrong verbiage now that i think about it, and do something like...

from models import equipment as equip equip.add.boots(some_dict) # add some new boots equip.table.boots(uSkaterUUID) # dump all rows of previous boots

Perhaps there is a better way of doing that instead of using classes to structure this??

I do reference the organization/structure a little bit more in #5

PaulWay · 2024-11-28T02:41:16Z

models/setup_db.py

+# to create, run: python -m models.setup_db
+
+# this script is a complete mess. PEP8 complains about the imports, which are all over the place.
+# note sure how to address this, if you're reading this - I sincerely wish it was better. :)


Is PEP8 complaining about imports that are never used in your code?

Yes, and via the documentation and verifying that I'm doing this as expected, the imports have to be after the import for "Base"

PaulWay · 2024-11-28T03:45:15Z

models/t_classes.py

+    date_end = Column(DateTime)
+    uSkaterUUID = Column(Integer)
+
+    def __init__(


Not sure why you put each argument on a separate line. It wastes vertical space. Any particular reason?

Only because they're usually long to put on one line, and when I break them up its much easier for my eyes to stack them vertically.

kami-nashi · 2024-11-29T05:20:32Z

models/__init__.py

@@ -0,0 +1,279 @@
+# coding=utf-8
+import pandas as pd


I may not be leaving pandas in this unless it brings a very specific advantage or feature. there are some test/learning functions towards the bottom of this file where i'm pulling joined table data directly to build some dataframes, but its unlikely to provide enough advantage to keep the library.

kami-nashi · 2024-11-29T05:23:04Z

models/__init__.py

+
+session = Session()
+
+


I've created a separate issue, #5 to consider how to go about structuring this file and the classes within it. Its very unlikely that the final product will resemble the current state since most of these statements were used to verify table and code structure while learning.

There are many opportunities for improvement here.

vkrizan · 2024-11-29T15:52:13Z

models/__init__.py

+                                    Ice_Time.uSkaterUUID == self.uSkaterUUID
+                                    ).where(
+                                        Ice_Time.uSkaterConfig == uSkaterConfig
+                                        ).scalar()


The indentation is strong here. Try to keep a vertical flow.

vkrizan · 2024-11-29T15:55:24Z

models/cyberconnect2.py

+db_url = os.environ['pgdb_host']
+db_name = os.environ['pgdb_name']
+db_user = os.environ['pgdb_user']
+passwd = os.environ['pgdb_password']


Beware of KeyErrors. You can use .get() with a default value instead of direct subscript with [].

In the case of requiring certain config variables, it would be good to check if they exist, and report back a proper error.

This is a really good point. I got so tied up in all of the new stuff that I completely forgot to go back and do basic error handling here.

vkrizan · 2024-11-29T15:56:46Z

models/cyberconnect2.py

+    coach_Fname = Column(String)
+    coach_Lname = Column(String)
+    coach_rate = Column(Float)
+    uSkaterUUID = Column(Integer)


Snake_case is usually preferred instead of camelCase, for variable/field names.

vkrizan · 2024-11-29T15:58:57Z

models/cyberconnect2.py

+        self.uSkaterUUID = uSkaterUUID
+
+
+class events_competition():


CameCase, with first character uppercased, is preferred for class names. Suggestions here: EventsCompetition.

vkrizan · 2024-11-29T16:00:59Z

models/__init__.py

+    def __init__(self, uSkateUUID):
+        self.uSkateUUID = uSkateUUID
+
+    def ice_cost(self):


If one method is expected and no state is required, then a class is not needed, and a function is preferred.

Suggestion here: def ice_cost(uSkateUUID)

vkrizan · 2024-11-29T16:06:13Z

models/__init__.py

+from models.cyberconnect2 import Session
+from models.ice_time import Ice_Time
+
+session = Session()


Ideally, creating any states, like a session here, should be done during main runtime. Meaning, that it should be created once it would be used. If this module (and it's submodules) would be imported, a session would be initialized as side-effect. This can create some unexpected behaviors, or confusion, for those wanting to use it as a library. Another strong point, for moving the state creation (initialization) to consumer (main runtime) is testability. Otherwise, creation of tests would require a monkey-patching.

This is just a thought and a general recommendation. There are a usual breakers of this, like creation of a module logger.

vkrizan · 2024-11-29T16:09:52Z

models/__init__.py

@@ -0,0 +1,38 @@
+# coding=utf-8


__init__.py should not contain code, but rather just imports (if needed). If submodules are not expected, then this can be a one file, a module, named modules.py.

vkrizan · 2024-11-29T16:12:07Z

models/__init__.py

+        for coach in coaches:
+            session.add(Coaches(**coach))
+            session.commit()
+        session.close()


A session is prematurely and unexpectedly closed here. Consider utilizing context-manager(with Session(...) as session), or pass the session (db connection) to the class/function.

vkrizan · 2024-11-29T16:14:19Z

models/__init__.py

+    def add_coaches(coaches):
+        for coach in coaches:
+            session.add(Coaches(**coach))
+            session.commit()


Consider using transactions, and commit after all required records are written.

Committing here would insert one record (Coaches) one by one, likely waiting the db to write it on disk before going to write another record (in the for loop).

kami-nashi added 2 commits November 20, 2024 13:11

Adding basic env requirements, updating readme

1463c15

initial work for models. currently allows total ice time and time on …

5410693

…a config to be pulled from postgress db

kami-nashi added this to the Phase 1 - Basic Database Initialization, Currency & Time Formatting Decorators milestone Nov 22, 2024

kami-nashi self-assigned this Nov 22, 2024

adding a class for importing costs from ice_time table

81800a8

kami-nashi linked an issue Nov 22, 2024 that may be closed by this pull request

Initialize a DB with default tables. #2

Open

kami-nashi added 8 commits November 22, 2024 14:11

updating with a better way to handle models and model files

1ed4b57

adding structures for creating tables from model files

d0668bf

rearranging some file structure, adding a 'setup' file to create tables

87ae93d

add coaches table model, updating cyberconnect2, adding class and fun…

81425f2

…ction for adding more coaches

updating models to allow for skate configs, boots, blades

ac9f386

updating docstrings for queries in models

0e57742

cleaning up cyberconnect2, moving all tables to files for easier grow…

5396cf5

…th in the future. updating any import references. lastly, adding new models to setup_db.py

global table data added to setup script.

51f2740

kami-nashi marked this pull request as ready for review November 28, 2024 01:25

kami-nashi mentioned this pull request Nov 28, 2024

Migrate queries from legacy code base #5

Open

PaulWay reviewed Nov 28, 2024

View reviewed changes

kami-nashi commented Nov 29, 2024

View reviewed changes

vkrizan reviewed Nov 29, 2024

View reviewed changes

kami-nashi and others added 2 commits November 29, 2024 15:43

adding time and currency decorators, some basic tests also (Skatetrax#6)

ae78cec

updating cyberconnect2 to be smater about env vars

ea591f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2024 _47b; Add db models #4

2024 _47b; Add db models #4

kami-nashi commented Nov 22, 2024 •

edited

Loading

kami-nashi commented Nov 28, 2024

PaulWay Nov 28, 2024

kami-nashi Nov 29, 2024

kami-nashi Nov 29, 2024

PaulWay Nov 28, 2024

kami-nashi Nov 29, 2024

PaulWay Nov 28, 2024

kami-nashi Nov 29, 2024

kami-nashi Nov 29, 2024

kami-nashi Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

kami-nashi Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

vkrizan Nov 29, 2024

2024 _47b; Add db models #4

Are you sure you want to change the base?

2024 _47b; Add db models #4

Conversation

kami-nashi commented Nov 22, 2024 • edited Loading

kami-nashi commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kami-nashi commented Nov 22, 2024 •

edited

Loading