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

Added vRY-Configurator #91

Merged
merged 7 commits into from
Jul 24, 2022
Merged

Conversation

tanishqmanuja
Copy link
Contributor

  • Used InquirerPy package
  • Can be called using cmd using "vry.exe --configure" or "python main.py --configure" while development

@OwOHamper
Copy link
Collaborator

OwOHamper commented Jul 12, 2022

otherwise LGTM, not tested

@OwOHamper OwOHamper closed this Jul 12, 2022
@OwOHamper OwOHamper reopened this Jul 12, 2022
@zayKenyon zayKenyon self-requested a review July 12, 2022 15:33
Copy link
Owner

@zayKenyon zayKenyon left a comment

Choose a reason for hiding this comment

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

Firstly, I ran vRY with no issue. Used the advanced config, applied the default value for PORT by simply hitting ENTER when prompted for a number; re-ran vRY to be greeted with the Firewall error message.

@tanishqmanuja
Copy link
Contributor Author

Will look into it today and update the PR.

@tanishqmanuja
Copy link
Contributor Author

Works fine for me now .. has anyone tested after recent commit?

@OwOHamper
Copy link
Collaborator

What about my review?

@tanishqmanuja
Copy link
Contributor Author

tanishqmanuja commented Jul 16, 2022

What about my review?

Fixed that problem.. its was due to string instead of int in port.

@tanishqmanuja
Copy link
Contributor Author

@OwOHamper, Have you made your review public.. only one review is showing here.
Screenshot_20220717-103339_Chrome~2

@OwOHamper
Copy link
Collaborator

image
hmmm I am not sure

@tanishqmanuja
Copy link
Contributor Author

Your review wasn't published ig.. cannot see it still on GitHub but image will do. Will push new commit for the suggestions

@OwOHamper
Copy link
Collaborator

You got any idea how I can publish it? lol

@tanishqmanuja
Copy link
Contributor Author

Same thing happened with decrypt so he can help . I never reviewed any PR myself so i cant help :(

@tanishqmanuja
Copy link
Contributor Author

Done refactoring. Plus since table mod PR is closed, can you remove this(pretty table from main.py) from source as it is unused now.
image

@OwOHamper
Copy link
Collaborator

it would still break you would need to create double if statement because it would check both values and if sys.argv[1] is undefined the first condition would be False but second one would still be checked. And also in the bare except you should print what exactly went wrong or initialize logs before it and log it. If there was some error we wouldn't have idea what it is

@tanishqmanuja
Copy link
Contributor Author

tanishqmanuja commented Jul 17, 2022

it would still break you would need to create double if statement because it would check both values and if sys.argv[1] is undefined the first condition would be False but second one would still be checked. And also in the bare except you should print what exactly went wrong or initialize logs before it and log it. If there was some error we wouldn't have idea what it is

AFAIK and operator is short-circuited in python and javascript that means second condition is only evaluated if first is true so double if is not required. I have tested calling main.py with and without arguments and both cases have worked.
As for the bare except, you are right about the situation but I being a js dev have little to no knowledge about the exceptions in python .. will need your help on that.

@D3CRYPT360
Copy link
Contributor

You got any idea how I can publish it? lol
opera_q3EJkDRSRW

@zayKenyon zayKenyon self-requested a review July 17, 2022 20:54
Copy link
Owner

@zayKenyon zayKenyon left a comment

Choose a reason for hiding this comment

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

Handling the input of no for proceed under src/configuator.py would lead to a better UX since it currently throws an error.

@tanishqmanuja tanishqmanuja requested a review from zayKenyon July 18, 2022 04:36
@zayKenyon zayKenyon merged commit dddc635 into zayKenyon:main Jul 24, 2022
@tanishqmanuja tanishqmanuja deleted the configurator branch July 27, 2022 09:24
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.

4 participants