Futura-Py / weather

Coming soon
MIT License
5 stars 1 forks source link

feat, refactor: settings window, code cleanup(?) #21

Closed im-coder-lg closed 2 months ago

im-coder-lg commented 2 months ago

The changes I made till now. Need to add user token system, test it out, create a release, and then we remove our token.

Closes #17

im-coder-lg commented 2 months ago

Let's merge this, and I'll try to slowly remove stuff like PYOWM.

im-coder-lg commented 2 months ago

Okay, this PR is ready for now. I'd like to change the config file from .txt to .json. We could just copy the file content in utils.py from TimerX and do this job. The lines in question are: https://github.com/Futura-Py/TimerX/blob/5020204afe37872a0f61f540d7b095c8713ea329/utils.py#L9-L42

Since the JSON file is read as a Python dictionary, we could just use key-value pairs. How does that sound? And I think we must divide the code base. Keep the GUI and API processes in one file, and the rest in another. :shrug: what do you guys think?

im-coder-lg commented 2 months ago

Sorry to disturb you guys, but you guys were there on #17; what do you think?

im-coder-lg commented 2 months ago

I'll change the things here. But long strings get separate lines due to the Black formatting.

Edit: It's more probable that this thing is due to AutoPEP8. Use the --diff tag on main.py and see if you wish to know. Can't we control it?

im-coder-lg commented 2 months ago

I think this is fine. Some things, like the button elements still have nearly 4-5 lines, but that wouldn't matter, since it's been like that since the start.

im-coder-lg commented 2 months ago

AFAIK my changes are done. To me, the PR is ready. But we really must control AutoPEP8 formatting.

In line 349 or something, there is a TODO. I think we finish that when we add more elements, and the window looks like a traditional window, rather than the size we have now. We could try making it into a rather balanced weather app, where it is conventional with some things and unconventional with other things.

https://github.com/Futura-Py/weather/blob/f63dd8663812a464de35eb58dd23f15dfb1d3263/main.py#L327

That's the line.

im-coder-lg commented 2 months ago

Sorry for this sudden surprise, guys, but I can't change more things today. I got some immediate work which I can only complete by tomorrow. I think the PR is ready, ad I approve for merging. If there are other things to change or add, we can do it after merging, if that's fine with all.

im-coder-lg commented 2 months ago

@Moosems Idea: rather than a configuration text file, what about a JSON config? Just an idea, very possible, what do you think?

Moosems commented 2 months ago

I think it's out of scope for the PR :)

Read my comment on the commit I just made by the way.

im-coder-lg commented 2 months ago

Hmm... Let's remove autopep8. And maybe we must divide the code into multiple files. And what do you think of a JSON config?

Moosems commented 2 months ago

Let's make those separate PR's but I agree with both