[GitHub] [incubator-superset] metaperl commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization

[GitBox <gi...@apache.org>]

metaperl commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization
URL: https://github.com/apache/incubator-superset/issues/9077#issuecomment-586375971
 
 
   > The root cause of that was a lack of shared understanding on the best code structure for Superset going forward
   
   1. What software architecture will you choose?
   1. What is your opinion of [the Clean architecture](https://pusher.com/tutorials/clean-architecture-introduction)
   
   > There are a number of modules that have grown organically over time to truly massive size. Often these modules contain utility functions, classes of multiple types, and a broad array of other functionality. These modules are very hard to work with.
   
   1. How do you plan to analyze this problem? Will you create a single mother ticket and a number of child tickets for each issue?
   1. How will you refactor all the tests in the current code base?
   
   > Business logic is frequently intermixed with display logic, making code reuse more difficult.
   
   1. Do you have specific examples of this problem?
   1. Isn't there a business reason for everything you display? how easy is it to separate the two?
   1. do you have a code reviewer? if so, who let these problems get into the codebase in the first place?
   
   > Circular dependency issues crop up regularly with the existing module design
   
   Would [Inversion of control or dependency injection](https://github.com/metaperl/python-oop/wiki/Dependency-Injection) help with this? If not, what will help with it?
   
   > Many modules have implicit dependencies on other modules which requires a specific load order
   
   This does not seem like much of a problem to me. Except that PyCharm (and other tools) run a module-reorder as part of their automatic code cleanup and this might prohibit something from loading.
   
   > There’s a substantial amount of app-specific code in shared modules
   
   I do not understand the problem: Are you saying there is **code duplication**? If the purpose of a module is to do something for the app, then it stands to reason it would have app-specific code, wouldnt it? I'm confused.
   
   > In some instances, particularly where there is pre-existing inheritance in Flask-AppBuilder, the inheritance chain can be over 10 classes deep. This can make understanding some classes very challenging.
   
   It can be challenging, but IMHO the code is well-written. I had to trace throgh 7 levels of OO inheritance here:
   https://github.com/apache/incubator-superset/issues/8695#issuecomment-559968158
   
   I would be curious to see how you would improve things and whether the problems is rooted in Flask itself and FAB and Superhave no choice but to bow to their Creator.
   
   > Organize modules function and back-end objects rather than products
   
   this phrase does not express a complete thought. I think there is some sort of grammatical error here. I have no idea what you are suggesting.
   
   > Avoid: SQL Lab, Explore
   
   One of these is a noun/product just like the things you prefer. The other is a verb.... What is unacceptable about SQL Lab?
   
   > class DashboardDAO(DataAccessObject):  def find_for_update(actor, dashboard_id): 
   
   Why is this preferred over a find_for_update in the class that `actor` belongs to? ditto for 
   `class DashboardDAO(DataAccessObject):  def update(dashboard, dashboard_params):` - this looks like a method that belongs to the Dashboard class.
   
   
   
   
   > 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org