[Issue 1325]: add python interactive console #1331
Conversation
rylew1
changed the title
| if isinstance(db, sqlalchemy.orm.scoped_session): | ||
| factories_module.db_session = db | ||
|
|
||
| variables["f"] = tests.src.db.models.factories |
There was a problem hiding this comment.
FYI - we might want to adjust where this utility lives - I made it so the factories are only a test component with the dependencies as dev only. We shouldn't be importing them into the src path ideally. This code won't ever run non-locally, but if you tried to do so, it would complain about missing packages.
I'm not sure if the tests folder is quite the right place, but I did put a utility script in tests/lib/seed_local_db.py - might be worth coming up with a happier approach.
There was a problem hiding this comment.
I'm open but would propose splitting that piece out to a separate PR.
I do think it would make sense to close the loop on the intercepting session errors though - so that we don't have to manually do a rollback when it gets to an error state
There was a problem hiding this comment.
I looked into the rollback automation, not finding a ton of approaches. Tried rolling back automatically when an error happens and that just cascades into other errors. In short, SQLAlchemy changed sessions to generally be made for handling a single set of transactions (great for an API, less so for an interactive console).
Without thinking through it much, the right approach probably involves lambdas/sessionmakers to automatically make new sessions when old ones error, but that's likely something that will take a while to figure out
There was a problem hiding this comment.
Want to just approve and merge then after fixing the repr stuff? We've got db queries and ability to play with factories - that's a win I think.
There was a problem hiding this comment.
Yeah, we can put that somewhere in the backlog, just adjust the repr bit and we can go from there
github-actions
bot
added
the
documentation
| return (f"<Opportunity(opportunity_id={self.opportunity_id}, " | ||
| f"opportunity_number='{self.opportunity_number}', " | ||
| f"opportunity_title='{self.opportunity_title}', " | ||
| f"agency='{self.agency}', category='{self.category}', ") |
There was a problem hiding this comment.
@chouinar - hope its ok to add repr methods on the db models so printing is a little bit cleaner - where before it was just printing the object and mem address (I was having call a vars(object) on it to see the values). Maybe there's a better way though than manually spelling it out for each model though.
| def __repr__(self) -> str: | ||
| return ( | ||
| f"<Opportunity(opportunity_id={self.opportunity_id!r}, " | ||
| f"opportunity_number={self.opportunity_number!r}, " | ||
| f"opportunity_title={self.opportunity_title!r}, " | ||
| f"agency={self.agency!r}, category={self.category!r})" | ||
| ) |
There was a problem hiding this comment.
There might be an easier to maintain way that we could use - I'm going to break a lot of this with an upcoming set of refactors to the models. The models already have a for_json method that converts a model to a dictionary. If we instead add a repr function like this to the base class, we wouldn't need to add this to every class:
def __repr__(self) -> str:
values = []
for k, v in self.for_json().items():
# Note that I see you added a format method to a lot of classes, probably call that on v here
values.append(f"{k}={v!r}")
return f"<{self.__class__.__name__}({','.join(values)})"
There was a problem hiding this comment.
yea the above worked - thanks!
There was a problem hiding this comment.
Added a couple factories in the ootb vars!
rylew1
requested review from
sumiat,
widal001 and
andycochran
as code owners
api/src/tool/console/interactive.py
Outdated
| variables["OpportunityFactory"] = OpportunityFactory | ||
| variables["OpportunitySummaryFactory"] = OpportunitySummaryFactory | ||
| variables["OpportunityAssistanceListingFactory"] = OpportunityAssistanceListingFactory |
There was a problem hiding this comment.
We have these all available on f.WhateverFactory already. Unless we want to add all of them like this (which is a lot to maintain) I'd prefer we just keep these in the f variable
There was a problem hiding this comment.
It was more for typing convenience for some of the main models - but sure let's stick to f for now
Summary
Fixes #1325
Time to review: 5 min
Changes proposed
make consoletarget that loads Python interactive consoleContext for reviewers
Screen.Recording.2024-02-26.at.5.48.26.PM.mov