Wizard feature v2 #220

Jacksonzhang34 · 2023-07-31T19:47:45Z

Issue #, if available:

Description of changes:
Version 2 of the wizard:

revision of the wizard, prefilled editable command line prompts
modularity: code is broken up into setter, getter, checker, Wizard, and data classes
code safety with the introduction of field enum class
Why is this change necessary:
change was necessary in order to address the feedback from the previous PR and ensure code safety, readability, and readiness for future changes
How was this change tested:
change will be tested

Any additional information or context required to review the change:

Checklist:

Updated the README if applicable
Updated or added new unit tests
Updated or added new integration tests
Updated or added new end-to-end tests
If your code makes a remote network call, it was tested with a proxy
You confirm that the change is backwards compatible

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2023-07-31T19:49:32Z

Codecov Report

Patch coverage: 51.61% and project coverage change: +13.06% 🎉

Comparison is base (5d87492) 77.24% compared to head (0eff94a) 90.30%.
Report is 2 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff                @@
##           development     #220       +/-   ##
================================================
+ Coverage        77.24%   90.30%   +13.06%     
================================================
  Files               40       45        +5     
  Lines             1472     1753      +281     
================================================
+ Hits              1137     1583      +446     
+ Misses             335      170      -165

Flag	Coverage Δ
integ	`74.78% <0.35%> (?)`
uat	`65.03% <0.35%> (-12.22%)`	⬇️
unit	`86.76% <51.61%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
gdk/wizard/commands/prompt.py	`0.00% <0.00%> (ø)`
gdk/commands/component/component.py	`86.66% <33.33%> (-13.34%)`	⬇️
gdk/wizard/commons/checkers.py	`70.58% <70.58%> (ø)`
gdk/wizard/commands/data.py	`81.81% <81.81%> (ø)`
gdk/wizard/commons/model.py	`83.54% <83.54%> (ø)`
gdk/wizard/commons/fields.py	`100.00% <100.00%> (ø)`

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

urvashijain18 · 2023-07-31T21:01:56Z

gdk/wizard/commands/prompt.py

+        # with open(self.project_config_file, "w") as config_file:
+        #     json.dump(self.field_map, config_file, indent=2)


Remove if not required anymore

urvashijain18 · 2023-07-31T21:05:42Z

gdk/wizard/commons/getters.py

Why do you have separate getter and setter files? In Java, bean class with attributes have getters and setters defined in same class. Is this is standard practice in python to have separate classes?

I'm not 100% sure if it is standard practice to have them in separate file. My thought process for to minimize the size of each file and separate out distinct task to hopefully make the code easier to read

Sorry to jump in unannounced...I get email notifications and thought I could contribute an opinion here:

Echoing Urvashi's point, I suggest designing around your data model rather than language concepts. For example, get author is really a behavior of project config. Wizard_getter#get_author vs ProjectConfig#get_author.

IMO, you get more discoverability and self-documentation with the latter approach. ProjectConfig clearly shows everything you can do with config, rather than having to inspect the whole Wizard_getter class to figure that out.

urvashijain18 · 2023-07-31T21:08:21Z

gdk/wizard/commons/checkers.py

+                "then",
+            }
+
+            if isinstance(schema, dict):


Use camel case for isInstance

For Python isinstance is a built-in function

urvashijain18 · 2023-07-31T21:08:53Z

gdk/wizard/commons/checkers.py

+                    elif key not in ignore_fields:
+                        traverse_schema(value, field_name)
+
+            elif isinstance(schema, list):


urvashijain18 · 2023-07-31T21:12:00Z

gdk/wizard/commands/prompt.py

+            # args = self.parser.parse_args(
+            #     [f'--{field}', input(f"Current value of the {require}{field} is {value}: ")]
+            # )


Remove if not required

urvashijain18 · 2023-07-31T21:15:02Z

gdk/wizard/commands/prompt.py

+                [
+                    f"--change_{build_or_publish}",
+                    input(
+                        f"Do you want to change the {build_or_publish} configurations? (y/n) "


Why do you want input from Customer? Even if they select "n" the fields will still be editable..right? Or you would simply exit the wizard? What is the behavior in each scenario?

The idea was to allow users to be prompted in an hierarchical manner. 'custom_build_command' and 'build_system' fall under the umbrella of build and "bucket", "region", and "options" all fall under the umbrella of publish, so if we first ask the user if they would like to change the build config or regions at all before prompting to change specific fields under their respective umbrellas. If the user respond 'n' the prompter would simply skip over the fields in build or publish.

saranyailla · 2023-08-01T23:14:39Z

gdk/wizard/tests/__init__.py

Remove this file along with the static and the integration folders.

urvashijain18 · 2023-08-06T01:10:32Z

gdk/wizard/commands/data.py

+
+class Wizard_data:
+    def __init__(self):
+        self.project_config_file = "/Users/jacksozh/Desktop/aws-greengrass-gdk-cli/gdk/wizard/static/default_config.json"


What if file exists? Also, you are specifying your local path which will not be present on customer's system.

urvashijain18 · 2023-08-06T01:15:15Z

gdk/wizard/commands/prompt.py

+            getter: Wizard_getter object
+            setter: Wizard_setter object


Update this.

urvashijain18 · 2023-08-06T01:30:27Z

gdk/wizard/commands/prompt.py

+        -------
+            string: the value for the field key "field"
+        """
+        require = "required " if required else "OPTIONAL "


Any particular reason why required is in small case and optional is in upper case?

My apologize, I should have been more consistent. I will change them both to upper case for readability and consistency.

urvashijain18 · 2023-08-06T01:36:26Z

gdk/wizard/tests/test_data.py

+    def test_init(self):
+        # self.project_config_file = _get_project_config_file()
+        # self.field_map = get_configuration()
+        # self.project_config = self.field_map["component"]
+        # self.component_name = next(iter(self.project_config))
+        # self.schema = self.get_schema()
+        pass
+
+    def test_get_schema():
+        pass


I do not understand this. Are you defaulting it to pass?

My apologizes, the test cases for this section is not yet complete. I just used pass for now

Add a comment then to inform reviewers that it will be updated in future CR.

saranyailla · 2023-08-07T18:20:02Z

gdk/wizard/commands/data.py

+import json
+
+
+class Wizard_data:


Rename all the class names to camel case - WizardData

saranyailla · 2023-08-07T18:20:27Z

gdk/wizard/commands/data.py

+        # self.project_config_file = _get_project_config_file()
+        # self.field_map = get_configuration()


Remove these.

saranyailla

We only need one module for Wizard. So, remove commands and commons modules and merge them into existing ones. We also don't need separate ones just for the wizard functionality testing. Do the same for them as well.

saranyailla · 2023-08-07T18:27:54Z

gdk/wizard/commons/checkers.py

+        return isinstance(input_value, str) and input_value in allowed_build_systems
+
+    def check_custom_build_command(self, input_value):
+        # input must be a non-empty string or a non-empty list of strings


Nit : Use """ """ for multi-line comments.

""" Line 1 Line 2 .... """"

saranyailla · 2023-08-07T18:29:10Z

gdk/wizard/commons/model.py

+
+    def get_author(self):
+        try:
+            return self.project_config[self.component_name][Fields.AUTHOR.value.key]


Use return self.project_config.get(self.component_name, default_value) instead, here and at every other place.

gotcha, I'm assuming that you mean that I should replace return self.project_config[self.component_name] with return self.project_config.get(self.component_name, default_value) and have a default value for component_name ?

saranyailla · 2023-08-07T18:35:52Z

gdk/wizard/commands/prompt.py

+        print("Exceeded maximum attempts. Assuming default response.")
+        return value
+
+    def change_build_or_publish(self, build_or_publish, max_attempts=3):


Why are we asking for these two together?

Oh its not asking both questions at once. It's asking about build or publish depending on the value of build_or_publish

I understand that nothing changes on the customer side but I don't think we need to club these two together in the code. Build and publish are two independent configs so coupling these two will make it hard for us to maintain and test the code in future.

Got it I will separate them out

Jackson Zhang added 2 commits July 20, 2023 22:50

feat: implementation of gdk wizard

21316fc

feat: gdk wizard version 2

89366d8

urvashijain18 reviewed Jul 31, 2023

View reviewed changes

Jackson Zhang added 2 commits August 1, 2023 10:49

chore: code clean up and documentation

a3ac25b

test: data and checkers

98e3b96

saranyailla reviewed Aug 1, 2023

View reviewed changes

gdk/wizard/tests/__init__.py Outdated

Copy link

Member

saranyailla Aug 1, 2023

Choose a reason for hiding this comment

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

Remove this file along with the static and the integration folders.

Jackson Zhang added 6 commits August 1, 2023 16:16

test: checkers

d34f733

chore: removed getter and setter files

0d24df8

feat: custom validation for each field

f8a501c

feat: data refactoring

6cc61ac

chore: refactoring prompt

189aaf2

test: more tests for checker

4382930

urvashijain18 reviewed Aug 6, 2023

View reviewed changes

saranyailla reviewed Aug 7, 2023

View reviewed changes

Jacksonzhang34 changed the base branch from main to development August 9, 2023 20:13

Jackson Zhang added 6 commits August 9, 2023 13:14

test: model, data, checker

6d655e4

chore: refactoring

1946b13

feat: exiting and exceptions

80428da

chore: file deletions

1941910

chore: removed data testing for now

887fffe

chore: included all wizard requirements

0eff94a

saranyailla closed this Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wizard feature v2 #220

Wizard feature v2 #220

Jacksonzhang34 commented Jul 31, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading

urvashijain18 Jul 31, 2023

urvashijain18 Jul 31, 2023

Jacksonzhang34 Jul 31, 2023

jcosentino11 Jul 31, 2023 •

edited

Loading

urvashijain18 Jul 31, 2023

Jacksonzhang34 Jul 31, 2023

urvashijain18 Jul 31, 2023

urvashijain18 Jul 31, 2023

urvashijain18 Jul 31, 2023

Jacksonzhang34 Jul 31, 2023

saranyailla Aug 1, 2023

urvashijain18 Aug 6, 2023

urvashijain18 Aug 6, 2023

urvashijain18 Aug 6, 2023

Jacksonzhang34 Aug 7, 2023

urvashijain18 Aug 6, 2023

Jacksonzhang34 Aug 7, 2023

urvashijain18 Aug 7, 2023

saranyailla Aug 7, 2023

Jacksonzhang34 Aug 7, 2023

saranyailla Aug 7, 2023

saranyailla left a comment

saranyailla Aug 7, 2023

saranyailla Aug 7, 2023 •

edited

Loading

Jacksonzhang34 Aug 7, 2023

saranyailla Aug 7, 2023

Jacksonzhang34 Aug 7, 2023

saranyailla Aug 7, 2023

Jacksonzhang34 Aug 7, 2023

		# with open(self.project_config_file, "w") as config_file:
		# json.dump(self.field_map, config_file, indent=2)

		# self.project_config_file = _get_project_config_file()
		# self.field_map = get_configuration()

Wizard feature v2 #220

Wizard feature v2 #220

Conversation

Jacksonzhang34 commented Jul 31, 2023

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcosentino11 Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saranyailla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saranyailla Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2023 •

edited

Loading

jcosentino11 Jul 31, 2023 •

edited

Loading

saranyailla Aug 7, 2023 •

edited

Loading