Consider using flet-routed-app for routing

[iron3oxide]

Since less (boilerplate) code and standardization should be aimed for, especially when it comes to routing, it might be worthwhile to refactor the app directory a bit in order to use the flet-routed-app library. Being the maintainer of said library, I will adapt it to the needs of tuttle wherever necessary.

[iron3oxide]

Small update on this: template routes are now integrated into the library, which was one of the main inhibiting factors. The next big one to tackle is that views don't receive a uniform set of parameters (excluding route parameters of course).

Since every one of the varying parameters (including TuttleParams) seems to be some member of the TuttleApp and each ViewBuilder in the library will automatically receive the RoutedApp as an attribute when it is added to said app, I suggest a refactor which would move the responsibility of passing app attributes/methods to the respective view into the build_view method of a custom TuttleViewBuilder class that inherits from the abstract ViewBuilder class.

This would either mean adding something like on_theme_change to TuttleParams as an optional attribute or creating a separate ViewBuilder implementation for each view that requires special parameters. Which one would you prefer?

[vlad-ed-git]

@iron3oxide add on_theme_change to TuttleParams.

[clstaudt]

Lacking much experience with Flutter / Flet, I don't have a strong opinion on this particular issue.

In general, I give a lot of importance to code readability and accessibility. We need to make life easy for open source contributors through the right architecture decisions. I'd ask you to consider if passing long lists of callbacks and big parameter sets to the various components could make the code base hard to navigate and understand.

[clstaudt]

@iron3oxide Do you want to keep us up to posted on your development progress by creating a branch and a draft pull request?

[iron3oxide]

Lacking much experience with Flutter / Flet, I don't have a strong opinion on this particular issue.

In general, I give a lot of importance to code readability and accessibility. We need to make life easy for open source contributors through the right architecture decisions. I'd ask you to consider if passing long lists of callbacks and big parameter sets to the various components could make the code base hard to navigate and understand.

In this instance, I'd argue it might make the code more readable, but certainly not less.

In general, I find that the current architecture could be more accessible. It took me some time, blog posts about the MVI pattern and a study of #76 to understand what was going on. Maybe a small collection of diagrams explaining the architecture could help? The alternative would of course be a thorough refactoring, which would slow down the project considerably.

@iron3oxide Do you want to keep us up to posted on your development progress by creating a branch and a draft pull request?

Will do, I'm starting to get out of the assessment/analysis phase.

[clstaudt]

In this instance, I'd argue it might make the code more readable, but certainly not less.

Yes, I am counting on your proposal to simplify some things @iron3oxide. Thanks for your effort.

@vlad-ed-git and I have so far worked out the current architecture and it's a work in progress. @vlad-ed-git has introduced the MVI pattern because

• it's typical in app development
• it offers clear separation of concerns, encapsulation and decoupling of view and backend layer (for example, the data layer would be easy to switch out from under the UI)

In general I think that it's a good idea to follow a MVI pattern. However, decoupling view and backend remains a challenge, and the MVI approach is neither trivial to apply, nor is it something that can be applied mechanically to yield perfect decoupling. You can still entangle UI and backend in subtle ways while building view, intent and data source classes.

I agree that the current architecture poses some challenges. And the UI code is relatively verbose. We may be able to make it more concise, but verbosity may also simply come from using Flet / Flutter.

Here is a wonderful quote from a talk on software architecture that describes my attitude:

I hate code and I want as little of it as possible in our product.

Documentation yes, but the architecture is also a work in progress so we haven't invested in writing it down.

The alternative would of course be a thorough refactoring,

Any specific ideas @iron3oxide ?

[iron3oxide]

To preface this: I found this blog post about the MVI pattern and this comparison to other architecture patterns to be very enlightening, though they are both a bit Android specific. While their defintions of MVI differ a bit, they seem to agree on the following:

1. a presenter is useful or even necessary to achieve separation of concerns and low coupling
2. the view should only be concerned with displaying the UI and propagating UI events
3. the view should be stateless

These principles/recommendations are not being followed right now in tuttle, which was certainly a reason for my initial confusion.

Any specific ideas @iron3oxide ?

I am by no means an expert on the MVP pattern, but I found it to be intuitive and helpful when used with flet, hence my MVP counter example project. If tuttle were to adopt a similar architecture (I'm totally on board with the MVI definition of the comparison blog post for example), it would have the following benefits:

• some code could be removed, because the presenter would have knowledge of the app instance, so e.g. TuttleViewParams wouldn't be needed anymore.
• the three principles mentioned above would be followed, which would mean better separation of concerns, testability etc.

The downsides would be:

• time and effort needed for refactoring
• more files (I take it you are not a fan of that @clstaudt), as each view module would need at least an extra presenter.py file and ideally also a protocols.py file if one doesn't want to cram the protocol classes into existing files

[clstaudt]

Thanks for the input @iron3oxide. Need some time to study that.

As for the pros and cons:

some code could be removed, because the presenter would have knowledge of the app instance, so e.g. TuttleViewParams wouldn't be needed anymore.

I struggle frequently with not having access to the app instance. And a helper class like TuttleViewParams is a bit of an architecture smell because it is a bag of unrelated things that need to be passed around, right?

time and effort needed for refactoring

Accessible architecture with sparse code is very important, there are a lot more features on the roadmap. So better sooner than later. Possibly after release of version 1.0beta to the non-Python-speaking public.

more files (I take it you are not a fan of that @clstaudt), as each view module would need at least an extra presenter.py file and ideally also a protocols.py file if one doesn't want to cram the protocol classes into existing files

Indeed not a fan of many files, especially if it's an unpythonic habit carried over from C++ and Java-like languages (extreme case: one class per file). If it gives the project a clear structure though adding more files can be justified.

[clstaudt]

2. the view should only be concerned with displaying the UI and propagating UI events
3. the view should be stateless
   These principles/recommendations are not being followed right now in tuttle, which was certainly a reason for my initial confusion.

@iron3oxide Did you find some code examples that go against these principles?

[iron3oxide]

I struggle frequently with not having access to the app instance. And a helper class like TuttleViewParams is a bit of an architecture smell because it is a bag of unrelated things that need to be passed around, right?

Agreed.

Accessible architecture with sparse code is very important, there are a lot more features on the roadmap. So better sooner than later. Possibly after release of version 1.0beta to the non-Python-speaking public.

Should I finish the PR related to this issue beforehand? Feels like waiting for the architecture to be settled might be wiser.

Indeed not a fan of many files, especially if it's an unpythonic habit carried over from C++ and Java-like languages (extreme case: one class per file). If it gives the project a clear structure though adding more files can be justified.

I'm not necessarily a fan of "one class per file" either, but I much prefer it to generically named files/modules with a lot of loosely related functionality inside. IMHO, the logic/inner workings of a project can be expressed in a reasonably verbose and well-organized file structure. If I search for a functionality, I'd rather spend 10 seconds navigating a large file tree to find the file I am 100% sure it is located in than spend the same 10 seconds or less searching through three possible files in a smaller file tree. Anyway, this is probably a matter of personal preference.

@iron3oxide Did you find some code examples that go against these principles?

Take projects/view.py for example:

• ProjectFiltersView very explicitly has a state
• ProjectEditorScreen and other Screens have instance variables that constitute state, e.g. self.tag
• None of the views in the file propagate events, they all have their own event handlers which sometimes contain business logic, e.g. ProjectEditorScreen.on_save()

[clstaudt]

Take projects/view.py for example:

• ProjectFiltersView very explicitly has a state
• ProjectEditorScreen and other Screens have instance variables that constitute state, e.g. self.tag
• None of the views in the file propagate events, they all have their own event handlers which sometimes contain business logic, e.g. ProjectEditorScreen.on_save()

@vlad-ed-git Can you comment?

[clstaudt]

Should I finish the PR related to this issue beforehand? Feels like waiting for the architecture to be settled might be wiser.

@iron3oxide For you to decide. If there are a lot of dependencies, probably yes.

[clstaudt]

ProjectEditorScreen and other Screens have instance variables that constitute state, e.g. self.tag

@vlad-ed-git I have to admit I don't understand why these instance variables exist. The only purpose seems to be as a temporary storage of the field values from the form, set as soon as the field is edited.

    def on_tag_changed(self, e):
        """Called when the tag input changes"""
        self.tag = e.control.value

When the submit button is clicked (on_submit), the intent is called with the instance variables instead of the field content. Do these on_changed event handlers for the fields need to exist? Can't the submit button event handler get the content of the fields?

[vlad-ed-git]

@clstaudt
You are correct. The on_change_calls are unnecessary, as we aren't checking the input as it is being typed. Add this to the refactoring issue, I will clear these calls from the form fields.

[clstaudt]

@vlad-ed-git Great, so here we can get rid of some code - always a reason to celebrate. Let's settle the architecture debate first though. Can you comment on the other points that @iron3oxide mentioned?

[vlad-ed-git]

@clstaudt and @iron3oxide
Okay, so both mvi and mvp (along with MVC, and MVVM) are heavily used in app development，with MVC and MVVM being the standard for iOS and Android respectively.

MVVM is Google's spin (and hence recommended approach for Android) on mvp to account for the lifecycle of the UI components, and mvi is also a spin on mvp as a response to reactive programming that is not android specific (even though it can and is used on Android).

When we started phase I development, I scanned for an architecture that we could adopt for this project. MVC was the initial consideration, because it is popular in the web world (actually the most popular because it is the oldest). But it's complexity made me search for a better candidate. I landed on mvi, because I am more familiar with it than MVP , because MVVM solves an issue we don't have with our desktop app, and because MVP is like MVI beta version.

However, all of these architectures truly shine when a project is large. It is hard to explain their usefulness (and in some cases even justify it) on a small to mid sized app. You have to imagine your project in a grown state, with multiple people working on separate layers and/or features.
So due to project size, coding style preferences, and the way flet is , we have had to modify (a lot) the architecture to fit our project.
We will do the same thing if we were to switch to MVP or any other architecture.

With that in mind, let's start with what we want and what an architecture offers:

1. We want to persist data.
   There are multiple ways to store data (relational database, files, in memory, remotely ...) and multiple ways each storage type can be implemented (files can be excel, csv, or .ics type. SQL databases have tones of ORMs , etc. ). A good architecture allows you to switch between the implementation of how a model is stored without having to make too many changes in the app.
   The solution these architectures offer is abstraction. You define a single abstract class that a storage type must implement.
   Example. File storage can define read file, write file. Then you can have implementations for each type excel, ics, and csv . The consumer of file storage just knows (from the abstract class) that read file method exists and it's signature .
   This means, for instance, the UI can define a file picker, and call a method -get calendar data from file- , without caring if the file is .xls, .xlsx, .csv or .ics . It just defines for the user that all these extensions are allowed and do not worry about how the actual reading for each is done (whatever library is used to parse a csv for instance).

2. We want to be able to query this data, and update it. Now, because there are multiple implementations of a single data source type, we need a way to know which to actually call.
   In the example above, we need a way to know that the file picked is a spreadsheet and thus the read data call should be made to an implemetantion of File storage that operates on spreadsheets.
   Also , often the data needed is not in the same format as the data to display. Example, the data can be a csv file but we are displaying a data frame.
   Also, often there is "complex business logic" to be handled before the query can be done or to the query result. A good example of this is making sure an invoice is rendered before it can be saved.

3. We want to be able to display the data, listen to and handle user events.

[vlad-ed-git]

Our aim is to do the above such that, these responsibilities are separated into neatly (as neatly as possible) defined layers. In a manner suitable for the scope of our project.

[vlad-ed-git]

So forget the names of the architecture for a second.
Let's focus on what are doing currently and where do we fail.

Currently,

1. On displaying data, we have a view (Ui). The view listens to an event, calls and event handler (which we have named intent) , gets data due to that event, and displays it.
2. Our event handler (intent classes) figures out which storage implementation (data source) to query for a specific event (and it's relevant data) and passes back the data that the Ui should display.
3. Our storage implementation handles the persistence of the data.
4. Each of the above should be as loosely coupled as possible. A good practical check is, if we were to swap one layer, then the other should have only a single line changed.

[vlad-ed-git]

Here is a good example. @iron3oxide @clstaudt

Consider the use case : Invoice Creation

The View displays a "create invoice" button.
When clicked, the intention of the user is to create an invoice, by specifying some fields of the invoice.
We could call the intent class with the event _on_new_invoice_button_clicked, only for the intent to return _display_new_invoice_form !
We can all agree, I am certain, that this would be bad.
Clearly NOT all events warrant a call to the intent class (this is something to keep in mind, as it is not always so obvious which event goes to an intent and which doesn't. Common sense is useful as always).

In this case, the UI itself handles the _on_new_invoice_button_clicked event, by displaying a form .
The user enters data into that form, clicks save.
Now we have another event, on_save_clicked .
And we need to do 3 things here.

First, we need to check if all the required fields are set and if the format is correct (if not, we display an error). Do we do this in the UI? Or let the intent class take care of that? It's not an obvious question as it may seem. HTML5 for instance will tell you if you click save without entering a required field, without waiting for JavaScript or the server to do so. However, some checks are complex requiring some storage access. Like checking if a unique field is actually unique. A common approach is to do some checking on the UI and let more complex checks get handled by the other layers.

Second, we need to render our invoice into a pdf. Definitely not something the UI should do. Infact, the UI should not even know about this step. From the perspective of the UI (or view, sorry I use these terms interchangeably) - the user clicked save, saving is what needs to be done. The intermediary rendering step is not even in it's radar.

Third, we need to actually save the invoice that has been created and rendered into our database. Also not something the UI should do.

So, we do some field checks, and forward the data to the intent, and wait for a result. The Ui must inform the user if creation is successful and must show an error message, if something goes wrong. In this way, it is the UI that defines the result type of the intent.

----- end of view part -------

Now we enter our intent layer.
The intent receives the invoice data entered by the user. It's result must specify if things go well or not (so perhaps a bool) . **It is worth noting that this signature seems to be defined by the UI. Which is to say, whoever writes the UI could get into the intent class code, add this signature (without truly implementing it), return False then proceed with the UI side.

def save_invoice(self, invoice_data) -> bool: #TODO Chris please implement this, thanks! return False
@clstaudt would later complain, rightfully so, that they should throw an Unimplemented Error here not return False!

The data that has passed some checks, so we don't need to do those specific checks again. And the rest of the checks (such as uniqueness of a field) are database specific.

We need to render this invoice. The intent should know WHO to call for rendering NOT actually how to render. This slight distinction might seem confusing but a good quick way to know is ...suppose library X is used to render an invoice . Is library X imported in the intent class? If yes， this is a violation of the rule (i.e. the intent KNOWS how rendering is done) . This is true even if the call to the library is as simple as libraryX.render_invoice.
Why?, because , if 4-5 weeks from now we decide library X sucks, and we want library Y, and library Y does not offer a simple one line call , then our intent would have to change a lot to perform rendering which is not it's purpose . @clstaudt and I have had some back and forth on this because he often feels like the intent is doing too little (and it is a correct observation, It does often seem so.).

Once rendering is done. Then saving is next and final. The intent must figure out which data source does this. And call it for saving. Now again, this might be confusing. An invoice model seems to be a prime candidate for SQL storage, certainly we won't store it in a file! Why do we say, figure out which data source? Well because SQL itself can be done in multiple ways. The intent should not have a - import some SQL ORM line at the top. It shouldn't start or end a db session or inherit from a SQLMixin (and all else that comes with sql ORMs . ).

At each step, something could go wrong. Now because the intent should not know the HOW only the WHO does what, the intent cannot know what exactly went wrong .but it can and must know that something went wrong (in order to return False).
We have arrived at yet another point of discussion between I and @clstaudt that is still ongoing. @clstaudt is in favor of try except blocks within the intent for this. You will see some part of the code where an intent method does a try - except and tries to figure out which exception was thrown . I disagree with this approach. I instead expect the data source to have those try except blocks . If you check the #145 issue, you will see that there is pending refactor to decide which approach to stick to.
But all in all, the intent should know if something goes wrong so it can tell the user.

--------- end of intent part ---------

The data source or sources. A great example of this is in timetracking feature. Here we do have multiple sources of the timetracking data. The cloud (itself consisting of many providers), the spreadsheet and the ics. I recommend looking at it. But back to creating the invoice.

Here a method should exist that defines how an invoice should be saved, perform those complex checks that may be needed, catch any exception that may occur (with my approach) and inform the intent if all goes well or if an error occured .

---- end of data source ----

If you begin to do this, you will notice patterns. Specifically, you will notice that almost always, the UI expects the intent to return some data, tell it if all went well, and if not, offer an error message. Thus we wrap all intent return types in the IntentResult (see core.intent_result) . Wrapping comes with a side effect though. Type hints for the data might get lost, since the wrapper receives ANY data type. Fortunately this is rare. Python allows us to do IntentResult[Optional[Invoice] for example.

Another pattern you will notice is that the data sources are also almost always expected to return some data , and if something goes bad, specify the log message. So once again, we wrap our result (my recommendation). And since this result is so similar to intentresult (not just by name but also what it will wrap), I re use IntentResult.
However, it is important to not that the data source will specify the log_message AND not the error message. The error message is what the Ui displays. The intent should be the one to specify this. The log_message is what the data source actually can know from the actual error. This is infact an example of data transformation. The intent can be said to "transform" the log_message data from the data source into the error message that user should see.

Example:
Suppose you attempt to create an invoice and an IntegrityError occurs. Now the data source will know this is an Integrity Error. And we want to log that this is an Integrity Error (along with all other info we might need for debugging or analytics ) . BUT in no way should the user see -an integrity error occured at ... ! The intent will log this message for us because all intents know the logger , then give the UI a more appropriate error message like - failed to save that invoice.

[vlad-ed-git]

@clstaudt and @iron3oxide I apologize for the lengthy comments above. Parts of which are possibly redundant. But I am attempting to offer not just an explanation of our MVI approach but also how we arrived at the codebase we have now (and the inconsistencies in some places) .

Here is a shorter code friendly explanation.

View:

on_save_btn_clicked(data):
      if data.is_empty:
           Tell user "um! It's empty!"
            return # can't save empty
      intent_result = self.intent.save_invoice_with_this_data(data)
     if not intent_result.was_intent_successful:
         Tell user intent_result.error_message
     else:
          Tell user "that went well"

Intent:

     save_invoice_with_this_data(data):
          intent_result = renderer.render(data)
          if not intent_result.was_intent_successful:
                  intent_result.error_message = "sorry user rendering failed"
                  intent_result.log_message_if_any() # whatever the heck went wrong with rendering, log that
                  return # can't proceed
          intent_result = data_source.save(data)
          if not intent_result.was_intent_successful:
                   intent_result.error_message = "sorry user, saving failed"
                   intent_result.log_message_if_any() # whatever the heck went wrong with saving, log that
                  return # can't proceed
         # if we get here, all is well .No data to transform so just return the result
       return intent_result

Renderer:

   Import some library for  data to pdf if needed
       render(data):
           try:
               #rendering ...
               return IntentResult(was_succesful= True)
           except BadDataException as B:
               return IntentResult (was_successful = False, log_message = f"exception raised at renderer.render {B} happened"
         ... 

Data Source:

  import some SQL ORM if needed
       save(data):
             try:
                # saving
                 return IntentResult (was_succesful= True)
             except IntegrityException as I:
                return IntentResult(was_succesful= False, log_message = f"an integrity error was raised while attempting to save {data}  ....")
            except ForeignKeyBadException as F:
                 return IntentResult(was_succesful= False, log_message = f"foreign key issue {F.__class__.__name___}...")
  ... 

[vlad-ed-git]

With this in mind, @clstaudt and @iron3oxide I don't think what we need is a move to a different architecture. Given that we took mvi, and adopted it to our purposes, whichever architecture we move to, we will also do the same and who knows if we would like the result so much. Infact I know enough about MVP to guarantee that we will end up with very similar code.

I suggest instead we focus on where there are inconsistencies with the approach we already decided on and why they exist,
agree on a standard,
stick to it (even at instances where it seems like a quicker way is available. Because understanding somewhat lengthy but standard code is far easier than understanding shorter code that breaks everything that you have been conditioned to expect),
and figure out the best way to communicate this standard to a new dev (because just saying we use MXY will never be enough. Perhaps they do not know that architecture OR they know it's standard implemetantion with all the bells and whistles that we have skipped).

[vlad-ed-git]

And to this end , here are pretty much the only modifications that I propose that @clstaudt will have strong feelings against, but hear me out.

1. Views for a feature should be separated into multiple files. UI code is verbose. Because of flutter's declarative UI approach. Placing every piece of UI that is related to a model in a single view file, makes for code that is very hard to navigate and read in my opinion. I honestly am amazed at the devs who are able to contribute so far. Could be a python thing, but I just don't see the benefit of having, for example, the code for the UI that creates a project, and one that displays a list of cards, and one that displays the project details screen, all in one file! These are literally three different screens! If you wish to edit some part of create form, why scroll around across hundreds of lines of fletControl(content=, padding=, margin= ）... from 2 other screens, looking for ProjectEditor screen , when you can just open ProjectEditor.py?

2. Zero try except blocks in intents and views.
   If I have to pick one, I will pick 2. It's just bad to expect the view or the intent to catch exceptions. The view is already too verbose. And the intent should not know THAT much about whatever operation needs to be done by the data source.

[iron3oxide]

Thanks, this is a great explanation of the current architecture, no need to apologize for its length!

I should probably have been a bit more precise about my ideas for refactoring. I didn't mean to propose to switch to MVP but to implement the MVI pattern differently than it is now implemented, that is with a presenter class between View and DataSource.

While rereading the first blog post I linked above, I noticed that the author marked it as outdated and linked to a very informative series of newer blog posts on the same topic. This graphic should explain what I envision.

I think a presenter class is the non-ephemeral connective tissue/broker between View and DataSource that is missing in the current architecture. It should be the place where event handlers are defined, even if the event handler just instructs the view to open a dialog and does nothing else. If you are wondering how that would look like in practice, have a look at this diagram.

On another note, said diagram made me think differently about models. They should indeed be "ViewState" and not merely a data source, which is how I used them up until now. I will update my MVP counter repo accordingly. Regarding tuttle though: implementing MVI in a useful way probably means doing the same, right now there are only data sources and no models.

EDIT: the last sentence sounds a bit ignorant, what I mean by it is that the benefits of MVI (one-directional control flow, reactive paradigm, single source of truth) would not come to shine as much without implementing this.

[iron3oxide]

And to this end , here are pretty much the only modifications that I propose that @clstaudt will have strong feelings against, but hear me out.

1. Views for a feature should be separated into multiple files. UI code is verbose. Because of flutter's declarative UI approach. Placing every piece of UI that is related to a model in a single view file, makes for code that is very hard to navigate and read in my opinion. I honestly am amazed at the devs who are able to contribute so far. Could be a python thing, but I just don't see the benefit of having, for example, the code for the UI that creates a project, and one that displays a list of cards, and one that displays the project details screen, all in one file! These are literally three different screens! If you wish to edit some part of create form, why scroll around across hundreds of lines of fletControl(content=, padding=, margin= ）... from 2 other screens, looking for ProjectEditor screen , when you can just open ProjectEditor.py?
2. Zero try except blocks in intents and views.
   If I have to pick one, I will pick 2. It's just bad to expect the view or the intent to catch exceptions. The view is already too verbose. And the intent should not know THAT much about whatever operation needs to be done by the data source.

I personally agree on both of these points. UI/flet controls code doesn't have to live in the view though. It could just as well be imported from something like a controls module that is in turn separated into useful submodules like cards, buttons etc . You can alway pass callbacks or refs to the UI functions or connect them after receiving the UI components.

[vlad-ed-git]

@iron3oxide could you explain the presenter you envision with a usecase, example: save_invoice. Across all the layers.

[clstaudt]

@vlad-ed-git and @iron3oxide thanks for the thorough description. However, I find this extremely hard to imagine and assess without concrete code examples. Ideally a reference implementation of one of the screens of the app in each of the proposed models.

[iron3oxide]

@vlad-ed-git @clstaudt I am working on an example, may take a bit though as I have university and work obligations coming up. You can check on some of my work regarding this in this project, where I try to create a library for flet apps that use an MVP-based architecture pattern (realistically any architecture pattern that aims to use immutable models and unidirectional control flow can use it at this stage though, especially MVI).

[iron3oxide]

@clstaudt @vlad-ed-git The library is usable now and can be seen in action in my counter example. I'm not sure yet, but I think it is definitely a sound implementation of MVP and maybe even MVI. Names and abbreviations aside, this or something very similar is the architecture I would propose.

If you need to see it with a tuttle screen, it will take a bit more time because I've got exams coming up, but I think the library readme and code + the counter app code should suffice to see what I envision. Didn't mean to ignore your wishes, just had to see it work first and what better way to do that than to rewrite ones own app.

Looking forward to hear your thoughts/feedback.

[clstaudt]

Thanks @iron3oxide, need some time to review your implementation.

[clstaudt]

I only have strong feelings against some specific patterns that were in the original design:

• Passing around an IntentResult containing data of type Any. I think this sabotages type checking and code readability.

    def load_timetracking_data_from_ics_file(
        self,
        ics_file_name: str,
        ics_file_path,
    ) -> IntentResult:

I tried to solve this by giving it a type variable, e.g. -> IntentResult[DataFrame].

• Two layers of IntentResult passing. That resulted in complicated procedural code for unpacking or re-packing IntentResult objects, many if-else branches. Hard to read, hard to get right, and looking "unpythonic". I think frequent bugs in this type of code agree with me - e.g. a semantically wrong error message displayed even though the error handling has not even become complicated yet.

        if not result.was_intent_successful:
            if is_updating:
                # recover old project
                old_project_result = self._data_source.get_project_by_id(
                    projectId=project.id
                )
                result.data = (
                    old_project_result.data
                    if old_project_result.was_intent_successful
                    else None
                )
            result.error_msg = "Failed to save the project. Please retry"
            result.log_message_if_any()
        return result

• DIY implementation of exception handling, printing and tracing. This log message is not something you want to write yourself. Let Python's exceptions and logging do the work for you instead of reinventing the square wheel. Complicates debugging, high risk of error hiding too because Exceptions are not properly propagated.

        return IntentResult(
           was_intent_successful=False,
           log_message="Un impemented error @TimeTrackingDataSource.load_timetracking_data_from_spreadsheet",
       )

[clstaudt]

UI code is verbose.

Not the right mindset. We just saw an example in which it was needlessly verbose, and I am sure there are more opportunities to make it more compact. 1050 lines of view code for a single app screen? Easier said than done, but I think we can do better.

(I've written UI code that is not verbose, namely with streamlit, which is unfortunately not applicable here. But highly recommended to try it out if you want the new experience of effortless UI development.)

[vlad-ed-git]

@clstaudt and @iron3oxide suggest moving the discussion on architecture to #145 issue. And note the standard you settle on there (the changes needed to be made).

[clstaudt]

How about moving it to Discussions?

I'm open to changing the architecture according to your suggestions, as long as I can see the practical implications of each proposal. I think that will need reference implementations of some part of the project. I'll have my list of concerns / antipatterns that I would like to see addressed or avoided in any architecture. And my goal is keeping the code pythonic.