Add `--telemetry` argument and ensure running without telemetry is possible #287

rajasegar · 2020-04-05T06:46:19Z

Fixes #282

tylerturdenpants · 2020-04-06T13:32:14Z

Let' get this bad boy over to github actions. 😉LGTM. Thank you

tylerturdenpants · 2020-04-06T13:35:01Z

Oh one thing, should we have tests for the new command line options? Just a thought.

rajasegar · 2020-04-06T14:14:26Z

@tylerturdenpants Yes, definitely I will add tests for non-telemetry use case

1. Update snapshot for no telemetry test 2. Add no telemetry test to CI 3. Tweaked transform to work without telemetry

rajasegar · 2020-04-07T05:29:59Z

@tylerturdenpants @Turbo87 @rwjblue Just a heads up, I have changed the logic here to make the tests passing, seem to have some doubts why it was working previously with telemetry and suddenly failing to work without telemetry.

ember-angle-brackets-codemod/transforms/angle-brackets/transform.js

Line 335 in 1bfbb5d

return !(KNOWN_HELPERS.includes(name) && config.helpers.includes(name));

The commit:
8e4fb8b#diff-0d3de8cb5f879d03639b6a16d0eddee0

tomwayson · 2020-04-22T20:43:52Z

This would fix #217 as well, right? If so, I'm a big fan.

tomwayson · 2020-04-23T14:07:36Z

transforms/angle-brackets/transform.js

@@ -317,7 +317,7 @@ function shouldIgnoreMustacheStatement(fullName, config, invokableData) {
    let strName = `${name}`; // coerce boolean and number to string
    return (isHelper || !isComponent) && !strName.includes('.');
  } else {
-    return !(KNOWN_HELPERS.includes(name) && config.helpers.includes(name));
+    return KNOWN_HELPERS.includes(name) && config.helpers.includes(name);


seem to have some doubts why it was working previously with telemetry and suddenly failing to work without telemetry.

I'm guessing that the tests haven't exercised this } else { block for a while?

I do think the previous logic was wrong, but I'm not sure about your change. Based on let isHelper = mergedHelpers.includes(name) || config.helpers.includes(name); from above, I would expect return KNOWN_HELPERS.includes(name) || config.helpers.includes(name);

I'm also wondering if there's additional logic inside the if (isTelemetryData) { block that needs to be captured like whether or not it's a component and whether or not the name includes a ..

Take this w/ a grain of salt b/c I haven't looked at the code outside of this fn, but it almost seems to me that you could remove isTelemetryData the if/else statement entirely and just execute what's in if block b/c of helpers || [] and components || [].

Agree, I think this should probably be an ||. 🤔

Agreed, will change that

README.md

rwjblue · 2020-04-23T14:24:19Z

README.md

@@ -23,14 +23,25 @@ It does not make a copy. Make sure your code is checked into a source control
 repository like Git and that you have no outstanding changes to commit before
 running this tool.

+```sh
+$ cd my-ember-app-or-addon
+$ npx ember-angle-brackets-codemod ./path/of/files/ or ./some**/*glob.hbs


We should ask for --no-telemetry to explicitly opt-out of telemetry gathering.

That makes sense given that you want telemetry to be the default use case.

However, personally I like the current choice to have --telemetry= replace --url=. To me if you don't provide --telemetry=, then --no-telemetry is not needed unless you wanted to use a default URL for telemetry - is that what you are suggesting?

I agree with @tomwayson, also having too many options/flags, for one thing, seems to bring in extra complexity

When using yargs you get --no-telemetry "for free" when you configure --telemetry to mean something. I think it is perfectly fine for the implementation to not require --telemetry or --no-telemetry (e.g. assume --no-telemetry if there is no --telemetry value provided) but our docs should explicitly pass the argument.

rwjblue · 2020-04-23T14:25:12Z

bin/cli.js

@@ -2,8 +2,12 @@
 'use strict';
 const { gatherTelemetryForUrl, analyzeEmberObject } = require('ember-codemods-telemetry-helpers');

+const argv = require('yargs').argv;


Should probably configure more parsing so we can give nicer error messages when the correct patterns aren't setup, also can emit --version and --help output "for free".

is that w/in the scope of this PR though?

Yes @tomwayson, the yargs parsing was brought as part of this PR, I will make the corrections accordingly

Ya, I'm fine if we punt the --help / --version bits to a different PR, but this PR should definitely define the specific arguments we do expect (e.g. --telemetry, and the positional values for the globs).

rwjblue · 2020-04-23T14:27:05Z

test/fixtures/without-telemetry/input/package.json

+    "broccoli-asset-rev": "^3.0.0",
+    "ember-ajax": "^5.0.0",
+    "ember-bootstrap": "^2.8.1",
+    "ember-cli": "~3.10.1",


Lets use a more modern ember-cli and app blueprint (either 3.16 or 3.18)

would those have any culry invocations to convert?

Yes, ideally the modern version shouldn't have any curly invocations, @rwjblue do you still want to change that.

Ya, I'd still prefer to use a new ember-cli. The templates that you check in aren't part of the app blueprint anyways so they would still have curlies or whatever you put in them.

I think it would be a good idea if the checked in app used a "mixed" layout, i.e. a classic app w/ at least a couple of pods artifacts to test. Like maybe a pods route that invokes a pods component?

That way the tests will verify that #217 is resolved.

I'd be glad to help w/ this. Maybe @rajasegar if you check in a classic 3.16/3.18 app w/ the tests working I can pull and generate the pods artifacts and push a commit w/ the updated fixture and tests?

rwjblue · 2020-04-23T14:29:24Z

transforms/angle-brackets/transform.js

@@ -317,7 +317,7 @@ function shouldIgnoreMustacheStatement(fullName, config, invokableData) {
    let strName = `${name}`; // coerce boolean and number to string
    return (isHelper || !isComponent) && !strName.includes('.');
  } else {
-    return !(KNOWN_HELPERS.includes(name) && config.helpers.includes(name));
+    return KNOWN_HELPERS.includes(name) && config.helpers.includes(name);


Agree, I think this should probably be an ||. 🤔

1. Introduced more processing with yargs 2. Changed readme to show telemetry option as default 3. Tweak the transform logic for shouldIgnoreMustache

rajasegar · 2020-04-24T02:23:10Z

Thanks @tomwayson @rwjblue for the review. Addressed the same in the latest commit.

tomwayson

This LGTM! Thanks @rajasegar!

[BREAKING CHANGE]: disabling telemetry by default

54d8318

rajasegar requested review from rwjblue, Turbo87 and tylerturdenpants April 5, 2020 06:46

rajasegar added 3 commits April 5, 2020 12:32

tests: add telemetry option in run-test

7f2d319

revert: remove telemetry option from execa in run-test

f5e2f7f

tests: fix integration tests with telemetry

d56fd4d

tylerturdenpants approved these changes Apr 6, 2020

View reviewed changes

tests: add tests for no telemetry option

8e4fb8b

1. Update snapshot for no telemetry test 2. Add no telemetry test to CI 3. Tweaked transform to work without telemetry

tomwayson reviewed Apr 23, 2020

View reviewed changes

rwjblue requested changes Apr 23, 2020

View reviewed changes

chore: address review comments

17b5a9c

1. Introduced more processing with yargs 2. Changed readme to show telemetry option as default 3. Tweak the transform logic for shouldIgnoreMustache

rajasegar changed the title ~~[BREAKING CHANGE]: disabling telemetry by default~~ [BREAKING CHANGE]: making telemetry optional Apr 24, 2020

tomwayson approved these changes Apr 24, 2020

View reviewed changes

tomwayson mentioned this pull request Apr 24, 2020

Trouble getting codemod to run on my project #217

Open

rwjblue approved these changes Apr 27, 2020

View reviewed changes

rwjblue added the breaking label Apr 27, 2020

rwjblue changed the title ~~[BREAKING CHANGE]: making telemetry optional~~ Add --telemetry argument and ensure running without telemetry is possible Apr 27, 2020

rwjblue merged commit 2912df0 into ember-codemods:master Apr 27, 2020

tomwayson mentioned this pull request Apr 28, 2020

add a note on skipping helpers when running w/o telemetry #310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--telemetry` argument and ensure running without telemetry is possible #287

Add `--telemetry` argument and ensure running without telemetry is possible #287

rajasegar commented Apr 5, 2020

tylerturdenpants commented Apr 6, 2020

tylerturdenpants commented Apr 6, 2020

rajasegar commented Apr 6, 2020

rajasegar commented Apr 7, 2020

tomwayson commented Apr 22, 2020

tomwayson Apr 23, 2020

rwjblue Apr 23, 2020

rajasegar Apr 24, 2020

rwjblue Apr 23, 2020

tomwayson Apr 23, 2020

rajasegar Apr 24, 2020

rwjblue Apr 24, 2020

rwjblue Apr 23, 2020

tomwayson Apr 23, 2020

rajasegar Apr 24, 2020

rwjblue Apr 24, 2020

rwjblue Apr 23, 2020

tomwayson Apr 23, 2020

rajasegar Apr 24, 2020

rwjblue Apr 24, 2020

tomwayson Apr 24, 2020

rwjblue Apr 23, 2020

rajasegar commented Apr 24, 2020

tomwayson left a comment

Add --telemetry argument and ensure running without telemetry is possible #287

Add --telemetry argument and ensure running without telemetry is possible #287

Conversation

rajasegar commented Apr 5, 2020

tylerturdenpants commented Apr 6, 2020

tylerturdenpants commented Apr 6, 2020

rajasegar commented Apr 6, 2020

rajasegar commented Apr 7, 2020

tomwayson commented Apr 22, 2020

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

rajasegar commented Apr 24, 2020

tomwayson left a comment

Choose a reason for hiding this comment

Add `--telemetry` argument and ensure running without telemetry is possible #287

Add `--telemetry` argument and ensure running without telemetry is possible #287