AF-3102 AF-2797 AF-1943 Update codemod to match final transition boilerplate #6

evanweible-wf · 2018-11-02T17:06:54Z

Description

Codemod for migrating to dart1+dart2 compatible boilerplate is complete with tests for all suggestors.
CI setup is complete as well, and the CI now uses PyInstaller to bundle the bin/ scripts (just one for now) into single-file executables. These are defined as build artifacts and can be manually attached to the GitHub releases when we cut them.

TODO

General cleanup
Finish tests
Update readme with contributing instructions
- Will do this separately
Add CI

Testing

CI passes
Verify that the executable produced as the exe_dart1_and_dart2 build artifact runs successfully (download it, chmod +x it, run it in a repo like over_react)

@Workiva/app-frameworks

aviary2-wf · 2018-11-02T17:07:37Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

rmconsole-wf · 2018-11-02T17:48:02Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

Code Review(s): #6
Release Image Tags:

Reviewers: corwinsheahan-wf, evanweible-wf, sebastianmalysa-wf

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: over_react_codemod 0.1.0
	Release Ticket(s): None

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Thursday, December 13 11:14 AM CST

corwinsheahan-wf · 2018-11-30T15:36:31Z

bin/over_react_migrate_to_dart1_and_dart2.py

+            codemod.run_interactive(query)
+
+    if check and num_changes_needed > 0:
+        print('Failed: %d changes needed.' % num_changes_needed)


corwinsheahan-wf · 2018-11-30T17:44:38Z

over_react_codemod/suggestors/generated_parts.py

+
+    if parent_library_uri:
+        # This file is a part file and has a parent library referenced by uri.
+


#nit extra new line

corwinsheahan-wf · 2018-11-30T17:45:46Z

over_react_codemod/suggestors/generated_parts.py

+    if generated_part_filename in existing_part_paths:
+        return
+
+    # Not necessary if we use a path to reference this parent from the generated part.


The builder will determine if there's a library directive, if not, it will use a part for it's part of directive, so I think we can remove this code

corwinsheahan-wf · 2018-11-30T17:46:55Z

over_react_codemod/suggestors/generated_parts.py

+    line_number_to_insert_parts = util.get_line_number_to_insert_parts(lines)
+    new_lines = [
+        '\n',
+        '// ignore: uri_does_not_exist, uri_has_not_been_generated\n',


I don't think we need the uri_does_not_exist tag here.

Works fine without it in Dart 1.

corwinsheahan-wf · 2018-11-30T18:02:58Z

over_react_codemod/util.py

+        return
+
+    end = matches[-1].start()
+    # -1 so a failed 'rfind' maps to the first line.


what is a failed 'rfind'?

See lines 181-183. All this does is pre-populate the newline_table with a mapping from -1 to 0 so that if the rfind() call below returns -1 (i.e. it didn't find the string), it will just map to the 0th line.

corwinsheahan-wf · 2018-11-30T18:22:30Z

over_react_codemod/updaters.py

+
+
+def insert_props_or_state_meta(lines, matches, prev_lines, next_lines):
+    for line in next_lines[:3]:


looks like this function could break if the consumer sorts meta anywhere other than below the class declaration?

Yeah, that is correct. There isn't a good way to find it other than this because class bodies and function bodies both use curly braces

This is one where using the dart analyzer would be much more robust

I'd suggest we drop a comment to tell consumers not to move that line

corwinsheahan-wf · 2018-11-30T18:24:24Z

over_react_codemod/updaters.py

+            continue
+
+        updated_line = line.replace(
+            'class %s' % class_name, 'class _$%s' % class_name)


Does this handle already private props/state class, for example _FooProps?

corwinsheahan-wf · 2018-11-30T18:25:50Z

over_react_codemod/updaters.py

+    updated = re.sub(pattern, 'class $%s' % class_name, combined)
+    updated_lines = util.split_lines_by_newline_but_retain_newlines(updated)
+    updated_lines.insert(
+        1, '// AF-3369 This will be made private once the transition to Dart 2 is complete.\n')


Are we 100% sure on that? I thought we would revisit the dart 2 final boilerplate at a later date. If we're not sure, we may want to not add this comment.

My thought right now is that we leave this in because I don't have a better idea at the moment. If we come up with a better solution down the road, that's fine, it just means this comment will be outdated

corwinsheahan-wf · 2018-11-30T18:27:32Z

over_react_codemod/updaters.py

+    combined = ''.join(lines)
+    factory_name = match.group(1)
+    updated = combined.replace(
+        '%s;' % factory_name, '%s = $%s;' % (factory_name, factory_name))


We should decide how we want to handle a private factory. Discussion here

corwinsheahan-wf · 2018-11-30T19:22:24Z

tests/suggestors/test_factories.py

+            start_line_number=3,
+            new_lines=[
+                '// ignore: undefined_identifier\n',
+                'UiFactory<FooProps> Foo = $Foo;\n',


we should have a test for a private factory

should that look like UiFactory<_FooProps> _Foo = _$Foo? Or maybe _$_Foo? Right now both the builder and this codemod script will output $_Foo, which is probably not what we want, since we'll want it to be private.

@greglittlefield-wf are there valid use cases for a private factory?

I've found at least this one in WSD: https://github.com/Workiva/web_skin_dart/blob/master/lib/src/ui_components/colorpicker/custom_color_input.dart#L8

- Support private factories (ensure generated initializer is also private) - Strip redundant underscore when renaming state and props classes - Add comment above static PropsMeta/StateMeta fields instructing users to leave them at the top of the class for regression checking - Remove unneeded `uri_does_not_exist` ignore comment - Remove code that handled adding a missing library directive - not needed

evanweible-wf · 2018-12-10T20:45:55Z

tests/suggestors/test_factories.py

+            start_line_number=3,
+            new_lines=[
+                '// ignore: undefined_identifier\n',
+                'UiFactory<_PrivateFooProps> _PrivateFoo = _$PrivateFoo;\n',


Is this what you were thinking @corwinsheahan-wf? Will it require a change to the builder?

Right now the builder generates the the private factory initialization as _$_PrivateFoo, since it simply takes the name of the factory and adds a _$ prefix, so yes this would require a builder update. I think I'm fine with either approach.

Ok, let's do that, I think it looks enough nicer to be worth it. _$_ is weird

evanweible-wf · 2018-12-10T20:46:40Z

tests/suggestors/test_props_and_state_classes.py

+            end_line_number=4,
+            new_lines=[
+                '@Props()\n',
+                'class _$PrivateFooProps extends UiProps {\n',


@corwinsheahan-wf Same question here, is this what you were thinking for private state/props classes? And will it require a builder change?

Here, I don't believe there would need to be a builder change. The builder will read the value of the props class and simply utilize that in it's generation. I'm fine with this as well. There will still be a _PrivateFooProps class which will retain the original intention that this is a private class (since the _$PrivateFooProps naming schema could have been from a public or private class).

corwinsheahan-wf

Just a couple of nits

corwinsheahan-wf · 2018-12-10T22:03:25Z

tests/suggestors/test_generated_parts.py

            new_lines=[
                '\n',
-                '// ignore: uri_does_not_exist, uri_has_not_been_generated\n',
+                '// ignore: uri_has_not_been_generated\n',


#nit could DRY these new_lines up

corwinsheahan-wf · 2018-12-10T22:05:02Z

tests/suggestors/test_props_and_state_mixins.py

            new_lines=[
                '@PropsMixin()\n',
                'class FooPropsMixin implements UiProps {\n',
+                '  // To ensure the codemod regression checking works properly, please keep this\n',


#nit could DRY this up as well

evanweible-wf · 2018-12-13T03:59:18Z

QA +1

Tested in over_react and web_skin_dart

@Workiva/release-management-p

rmconsole-wf · 2018-12-13T03:59:39Z

@evanweible-wf I will not merge this because:

'No major decrease to number of internal dependencies' is False

rmconsole-wf · 2018-12-13T17:14:04Z

+1 from RM

Update codemod to match final transition boilerplate.

f43077f

Remove broken imports.

8496660

rmconsole-wf added the Merge Requirements Unmet label Nov 2, 2018

evanweible-wf and others added 11 commits November 2, 2018 12:56

Properly type the static meta fields.

1b27b05

add meta getter and fix parts not working

7a4d9ae

fix some bugs and formatting

d584e0d

small tweaks

17d19ca

Update (Props|State)Mixin meta suggestor.

e2a7474

Update after testing in WSD

eeb3dba

Address more edge cases, add tests for all suggestors.

d160b5d

More edge cases!

3fc38b0

overReact.g.dart --> over_react.g.dart, use actual AF ticket

29e2024

Remove dart tests in favor of python tests.

9b16be3

Setup CI and bundle distributable executable.

414bdfc

evanweible-wf changed the title ~~AF-2797 AF-1943 Update codemod to match final transition boilerplate~~ AF-3102 AF-2797 AF-1943 Update codemod to match final transition boilerplate Nov 30, 2018

corwinsheahan-wf requested changes Nov 30, 2018

View reviewed changes

evanweible-wf commented Dec 10, 2018

View reviewed changes

corwinsheahan-wf approved these changes Dec 10, 2018

View reviewed changes

evanweible-wf added 2 commits December 11, 2018 09:43

DRY up tests

b2c6f8c

Better handling of private props/state classes naming.

6feb972

sebastianmalysa-wf approved these changes Dec 12, 2018

View reviewed changes

corwinsheahan-wf approved these changes Dec 12, 2018

View reviewed changes

rmconsole-wf added the RM Ready label Dec 13, 2018

rmconsole-wf removed the Merge Requirements Unmet label Dec 13, 2018

rmconsole-wf added the Merge Requirements Met label Dec 13, 2018

rmconsole3-wf merged commit 6ef11a4 into master Dec 13, 2018

rmconsole3-wf deleted the round2 branch December 13, 2018 17:14

rmconsole-readonly-wk mentioned this pull request Jan 23, 2019

AF-3863 Release over_react_codemod 1.0.0 #15

Merged


		if parent_library_uri:
		# This file is a part file and has a parent library referenced by uri.



		def insert_props_or_state_meta(lines, matches, prev_lines, next_lines):
		for line in next_lines[:3]:

AF-3102 AF-2797 AF-1943 Update codemod to match final transition boilerplate #6

AF-3102 AF-2797 AF-1943 Update codemod to match final transition boilerplate #6

Uh oh!

Conversation

evanweible-wf commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Testing

Uh oh!

aviary2-wf commented Nov 2, 2018 • edited by aviary3-wk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Insights

Action Items

Uh oh!

rmconsole-wf commented Nov 2, 2018 • edited by rmconsole3-wf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Requirements Met ✅

General Information

Additional Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corwinsheahan-wf Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corwinsheahan-wf Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanweible-wf Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corwinsheahan-wf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

evanweible-wf commented Nov 2, 2018 •

edited

Loading

aviary2-wf commented Nov 2, 2018 •

edited by aviary3-wk

Loading

rmconsole-wf commented Nov 2, 2018 •

edited by rmconsole3-wf

Loading

corwinsheahan-wf Nov 30, 2018 •

edited

Loading

corwinsheahan-wf Dec 10, 2018 •

edited

Loading

evanweible-wf Dec 10, 2018 •

edited

Loading

corwinsheahan-wf left a comment •

edited

Loading