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

bump requirements, fix tests and add get tentacles dict #140

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

techfreaque
Copy link
Contributor

No description provided.

@GuillaumeDSM GuillaumeDSM self-assigned this Sep 7, 2022
@Herklos Herklos enabled auto-merge (rebase) September 7, 2022 18:50
@GuillaumeDSM
Copy link
Member

auto-merge was automatically disabled September 13, 2022 11:44

Head branch was pushed to by a user without write access

@techfreaque
Copy link
Contributor Author

Once this test is passing, we can merge it ! https://github.com/Drakkar-Software/OctoBot-Tentacles-Manager/runs/8235204721?check_suite_focus=true#step:5:173

Looks like its an issue happening only on linux, here on my windows its passing all tests.
Do you know how to fix it? If not I'll set up linux enviroment to debug it.
And also I fixed some tests that were not working in the last commit

@@ -26,6 +26,26 @@ def get_installed_tentacles_modules() -> set:
return set(tentacle for tentacle in loaders.get_tentacle_classes().values())


def get_installed_tentacles_modules_dict() -> dict:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will it be used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be used with the react ui to manage installed tentacles
meanwhile i moved it to the react ui

I also updated the pull request - I removed create folder if it doesnt exist as its already fixed in master

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't you use get_installed_tentacles_modules to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its returning as a set and tentacle_type is an object which doesnt work with json without changing it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not converting to json in client (caller) side?

@techfreaque techfreaque changed the title [Exporter] create folder if it doesnt exists instead of raising an error bump requirements, fix tests and add get tentacles dict Nov 12, 2022
Comment on lines 166 to 170
def get_installation_context_octobot_version() -> dict:
setup_config = configuration.TentaclesSetupConfiguration()
available_tentacle = util.load_tentacle_with_metadata(constants.TENTACLES_PATH)
setup_config.fill_tentacle_config(available_tentacle, constants.TENTACLE_CONFIG_FILE_NAME)
return setup_config.installation_context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@techfreaque
Copy link
Contributor Author

regarding your questions:

  • [Tests] fix unknow octobot version tests
    why a new api ? if it's just for tests, we should create a test method
  1. I moved it to tests as its only required there
  • [Api] get installed tentacles as dict
    why get_installed_tentacles_modules_dict ? (keys should be constants)
  1. I want to get the data ready to convert to json without needing to remove/modify objects in it. And I replaced it with an enum

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.

3 participants