Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add application states #1974

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Conversation

Artem-Safronov
Copy link

@Artem-Safronov Artem-Safronov commented Dec 7, 2024

Description

Hello. While writing the test for launching the application, I encountered the need to check the application's active state. I suggest adding two states for the application: "stopped" and "running".

Type of change

Please delete options that are not relevant.

  • New feature (a non-breaking change that adds functionality)

Checklist

  • My code adheres to the style guidelines of this project (scripts/lint.sh shows no errors)
  • I have conducted a self-review of my own code
  • I have made the necessary changes to the documentation
  • My changes do not generate any new warnings
  • I have added tests to validate the effectiveness of my fix or the functionality of my new feature
  • Both new and existing unit tests pass successfully on my local environment by running scripts/test-cov.sh
  • I have ensured that static analysis tests are passing by running scripts/static-analysis.sh
  • I have included code examples to illustrate the modifications

@Artem-Safronov
Copy link
Author

@Lancetnik
Hello, will there be an opportunity to take a look? I would like to understand whether it’s worth waiting for the states or if I should consider approaching this case differently.

@Lancetnik
Copy link
Member

@Lancetnik Hello, will there be an opportunity to take a look? I would like to understand whether it’s worth waiting for the states or if I should consider approaching this case differently.

Oh, sorry, missed that one
Unfortunately, we can't merge it to main until 0.6 release. It is in the final stage (mypy and docs polishing) and a don't wont to add new features to main. If you want - you can add this feature to 0.6 branch. It has already some kind of state for application, so it should be easy to add.

@Artem-Safronov Artem-Safronov changed the base branch from main to 0.6.0 December 11, 2024 06:19
@Artem-Safronov
Copy link
Author

@Lancetnik Hello, will there be an opportunity to take a look? I would like to understand whether it’s worth waiting for the states or if I should consider approaching this case differently.

Oh, sorry, missed that one Unfortunately, we can't merge it to main until 0.6 release. It is in the final stage (mypy and docs polishing) and a don't wont to add new features to main. If you want - you can add this feature to 0.6 branch. It has already some kind of state for application, so it should be easy to add.

I changed the pull request to 0.0.6. I see that the StartAbleApplication class has its own state, but it relates to dependency injection. I suggest considering renaming it to _di_state.

@Lancetnik
Copy link
Member

@Artem-Safronov I designed this state is a single state for the app, so we should extend it by your functional instead of renaming

@Artem-Safronov
Copy link
Author

@Artem-Safronov I designed this state is a single state for the app, so we should extend it by your functional instead of renaming

Ok. I was confused that this is an instance of the DIState class. Then I will extend it with the attribute state.

@Lancetnik
Copy link
Member

is an instance of the DIState class

It is not a problem, we can add special ApplicationState, providing di_state and application-specific states as well

@Lancetnik
Copy link
Member

Also, I am not sure about Enum - it has just a two fields. Could it be just a boolean flag?

I imagine smth like

class ApplicationState(ABC):
     def __init__(self, di_state) -> None:
         self._di_state = di_state

     @property
     def running(self) -> bool: ...

class BasicApplicationState(ApplicationState):
     @property
     def running(self) -> bool:
         return False

class RunningApplicationState(ApplicationState):
     @property
     def running(self) -> bool:
         return True

@Lancetnik Lancetnik merged commit b84c032 into airtai:0.6.0 Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants