Louis-Dreyfus-Company / geeservermap

Interactive map for Google Earth Engine in python
MIT License
3 stars 0 forks source link

refactor: use modern python development pardigm #18

Open 12rambau opened 10 months ago

12rambau commented 10 months ago

TODO

I already reviewed the helper.py file and I started to look at the layers.py one. There is a lot of duplicated code. As refactoring is a painful process I would like to make sure that this is needed. If not let me know and we'll try to get rid of the one that are done twice.

@rodrigo-ldc could you help me on this?

rodrigo-ldc commented 10 months ago

I guess by modern python development paradigm you mean using dataclasses. To be honest, I've never seen it before, looks cool but it's something I need to get used to. Getting rid of __init__ is a big step for me.

rodrigo-ldc commented 10 months ago

When I was about to finish reviewing helpers.py I realized that many of the functions there were replaced by methods in layers.VisParams. I'm sorry @12rambau if you find this confusing, this was a WIP and I put it in GitHub as it was 'cos I wanted the rest of the team to benefit from it, and use it in development. Should've clean it up before uploading, what I was in a hurry and decided to do it after

12rambau commented 10 months ago

I guess by modern python development paradigm you mean using dataclasses. To be honest, I've never seen it before, looks cool but it's something I need to get used to. Getting rid of __init__ is a big step for me.

If you look at the PR I also changed numerous things that were not available prior to Python 3.7 one-liner extention of dict, no init files, dataclasses so that's a bit more than just replacing files. Also note that my objective is to change nothing on how it is working

rodrigo-ldc commented 10 months ago

Yeah, don't worry, improvements in behavior are also welcome 😃 I even have a todo list, like .getBounds(), .setBounds, centerObject! In fact, I'm thinking in refactor (improve) the messaging behavior, 'cos now it's only working for adding layers to the map, but my general idea is that the message should contain the method that it'll call and the params that will use for that method, what do you think?

12rambau commented 10 months ago

Yeah, don't worry, improvements in behavior are also welcome 😃 I even have a todo list, like .getBounds(), .setBounds, centerObject! In fact, I'm thinking in refactor (improve) the messaging behavior, 'cos now it's only working for adding layers to the map, but my general idea is that the message should contain the method that it'll call and the params that will use for that method, what do you think?

I think all these ideas are good ideas but they are out of scope for this PR. Could you open individual issues for each one of them so we can discuss implementation on case by case ?