Refactored consumption.py

kaestro commented 4 months ago

Haven't tested whether this runs for I haven't set up sqlite and don't know how to,

but I've started from refactoring consumption.py into separate files.

Wonder this type of programming won't be of your type of style.

If any suggestions and problems let me know.

BuildWithData commented 4 months ago

my plan is to do this eventually, but not right now bcs:

your refactor goes in the way I think we gonna do this eventually, but rather than functions I'd go with a ORM like in django.

So we gonna have a class where we gonna define the interface and all the plumbing methods:

class Table(ABC):

         def read():
             """return all records from table"""

         def create():
              """materialize empty table"""

         def write():

and then all tables will inherit from this and define the logic only, for example:

class Products(Table):

     def __init__(self):
           self.schema = """
                ticker      TEXT    NOT NULL    PRIMARY KEY,
                type        TEXT    NOT NULL    CHECK (type in ('etf', 'etp')),
                firm        TEXT    NOT NULL,
                country     TEXT                CHECK (country in ('USA', 'CANADA'))
           """

so this won't be merged, but you can work on other tasks and keep this part of the repo like in this pull request if you like it. This is just a refactor, so you should get the same exact result I get when tables are created and populated.

Also added a few comments to your code

BuildWithData commented 4 months ago

Haven't tested whether this runs for I haven't set up sqlite and don't know how to,

sqlite db does not need any set up, that's why I have chose it. That said you can change its default settings, added here a comment from where you can start working this out

Again, if you struggle with this, just let me know what is blocking you... on the other hand you can work on another issue, if this one is boring or too hard to fix

let me know :)

kaestro commented 4 months ago

Thanks for the comments. I didn't know that sqlite doesn't need any setups at all. That was why I was wondering where I can find the db from and reason I was refactoring the code because I'm not that good at reading code

I think I wasn't that blocked on the issue yet, but if it's in a hurry that it's really fine if you finish the work on this and I will work on another issue.

I won't try to refactor on those SQL embedded codes from now on, thanks for letting me know that you're planning on shifting towards orm

kaestro commented 4 months ago

@BuildWithData

BTW, for its my first time working on open source project I'm not used to how I can be of communicating yet.

I thought sending pull request was only way of showing you what my idea was. Is there any better way of expressing my situation? it would be of real help. Thank you

BuildWithData commented 4 months ago

I thought sending pull request was only way of showing you what my idea was. Is there any better way of expressing my situation? it would be of real help. Thank you

Look I am no expert at all, so don't worry, just ask whenever your are in doubt :)

coming to your question, I think it depends on the project itself, some people like it one way, others like it another way... it could also depend on project size, for example if many devs are working on a project, I think having a pull request always linked to an issue is a good practice...

That said, I like the idea of draft pull request, you can learn more here

BuildWithData commented 4 months ago

Thanks for the comments. I didn't know that sqlite doesn't need any setups at all. That was why I was wondering where I can find the db from and reason I was refactoring the code because I'm not that good at reading code

I think your was a smart move, reading code is one thing, writing/refactoring it just gives you a better understanding

I think I wasn't that blocked on the issue yet, but if it's in a hurry that it's really fine if you finish the work on this and I will work on another issue.

yeah, if you could solve the issue on decimal digits, that would be great, but fell free to pick any other issue... I'd suggest to start with this https://github.com/BuildWithData/BTC-ETF-Tracker/issues/14, this is high priority and by working on it you would get a better understanding of the whole codebase

BuildWithData / BTC-ETF-Tracker

Refactored consumption.py #40