-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add support for multiple servers #73
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all it is really good and just some minor features/cleanup and one bug fix. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation but you can remove this file as there is already a README.md in this project.
You can add however your explanation in the actual README.md for example in Chapter 3 😃
@@ -41,5 +43,5 @@ | |||
# no arguments were used so start pluGET console | |||
clear_console() | |||
print_logo() | |||
check_for_pluGET_update() | |||
# check_for_pluGET_update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the updatecheck for pluGET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use indention with 4 spaces instead of tab :)
|
||
def check_config() -> None: | ||
""" | ||
Check if there is a pluGET_config.yml file in the same folder as pluget.py and if not create a new config | ||
and exit the programm | ||
""" | ||
if not os.path.isfile("pluGET_config.yaml"): | ||
if not os.path.isfile("config.yaml"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to change this also in the README.md guide
if config.connection not in accepted_values[0]: | ||
console.print(f"Error in Config! Accepted values for key 'Connection' are {accepted_values[0]}", | ||
style="bright_red") | ||
exit_afterwards = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line and instead of if else just sys.exit()
to not make the code this abomination of indented lines :D
@@ -110,7 +124,7 @@ def get_input() -> str: | |||
:returns: Optional parameter | |||
""" | |||
input_command = None | |||
print("\n'STRG + C' to exit") | |||
# print("\n'STRG + C' to exit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this self explanatory or why did you remove it?
server_list = {} | ||
active_server = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add code comments which explain it a little bit more?
# from src.handlers.handle_config import config_value | ||
from src.utils.console_output import rich_print_error | ||
from src.handlers.handle_sftp import sftp_create_connection, sftp_upload_server_jar | ||
from src.handlers.handle_ftp import ftp_create_connection, ftp_upload_server_jar | ||
# from src.handlers.handle_sftp import sftp_create_connection, sftp_upload_server_jar | ||
# from src.handlers.handle_ftp import ftp_create_connection, ftp_upload_server_jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the commented lines as they are not needed anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this file to test_all_configured_servers
or something like that?
def check_requirements() -> None: | ||
""" | ||
Check if the plugin folders are available | ||
""" | ||
config_values = config_value() | ||
match config_values.connection: | ||
case "local": | ||
check_local_plugin_folder(config_values) | ||
case "sftp": | ||
sftp_create_connection() | ||
case "ftp": | ||
ftp_create_connection() | ||
return None | ||
# def check_requirements() -> None: | ||
# """ | ||
# Check if the plugin folders are available | ||
# """ | ||
# match config_values.connection: | ||
# case "local": | ||
# check_local_plugin_folder(config_values) | ||
# case "sftp": | ||
# sftp_create_connection() | ||
# case "ftp": | ||
# ftp_create_connection() | ||
# return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just delete it
Hey @Sowgro,thank you so much for your PR. :) |
I am glad you got a chance to look at this. I will look through your comments later this week. This was the first time I ever wrote code in python, so my apologies for any weird formatting. |
This adds support for multiple servers with a switch server command
As well as a new config file config.yaml, similar to what was proposed in #58