add --rename option to rename declarations and references #47

frapontillo · 2014-07-16T18:01:06Z

The --rename option takes a string as input, treating every couple, separated by a space,
as a find-replace option.
For example, --rename "$theService $myService theFactory myFactory" will replace $theService with $myService and theFactory with myFactory in both the declaration and in any annotation that will be added by ng-annotate. The actual use of the provider won't be changed.

Closes #44

The `--rename` option takes a string as input, treating every couple, separated by a space, as a find-replace option. For example, `--rename "$theService $myService "theFactory myFactory"`will replace `$theService` with `$myService` and `theFactory` with `myFactory` in both the declaration and in any annotation that will be added by ng-annotate. The actual use of the provider won't be changed. Closes olov#44

frapontillo · 2014-07-16T18:02:47Z

ng-annotate-main.js

@@ -218,7 +218,7 @@ function matchRegular(node, ctx) {
    const args = node.arguments;
    const target = (is.someof(method.name, ["config", "run"]) ?
        args.length === 1 && args[0] :
-        args.length === 2 && args[0].type === "Literal" && is.string(args[0].value) && args[1]);
+        args.length === 2 && args[0].type === "Literal" && is.string(args[0].value) && [args[0], args[1]]);


The main difference is here, where I return another target, besides the defined function, that is the given provider name.

olov · 2014-07-16T18:21:02Z

Thanks for the PR! I'll review it and provide feedback as soon as I can (may take a day or two).

frapontillo · 2014-07-16T18:26:27Z

Thanks! This was mainly needed to solve incompatibilities between angular-ui-bootstrap ang angular-strap, as they both declare their own $modal and $tooltip.

frapontillo · 2014-07-17T08:13:15Z

While trying to build I realized that nginject-comments.js was having issues due to my modifications of the stringify function. Also, the compiler complained about the use of var, so I replaced the occurrences with let.

mgol · 2014-07-17T21:15:29Z

ng-annotate.js

@@ -126,6 +131,15 @@ function runAnnotate(err, src) {
        });
    }

+    if (config.rename) {
+        let flattenRename = config.rename.split(" ");
+        let renameMap = {};


Most of added lets should be replaced with consts. Even this one (const is shallow constant, you can still mess with keys).

mgcrea · 2014-07-27T18:45:11Z

👍

olov · 2014-07-27T21:41:31Z

Overall I think this is fine and I'm willing to merge it as an experimental feature. Because renaming is not really a core feature of ng-annotate, it may be removed in the future, in which case I'll try to make it possible to implement the same thing with a plugin. Also see #42. Make sure that you're confident with this.

Before merging, it needs tests. Create a tests/rename_original.js and tests/rename_renamed.js and the necessary code in the runner. Also please change all your let to const, then only make things let when the variable binding changes (v8 will tell you if you're unsure, so will defs in build.sh).

frapontillo · 2014-07-27T21:50:31Z

Achieving the same thing with a plugin would be awesome.
I will try to complete the PR in the next couple of days (I hope), as I'll be AFK the first half of August.

@smrq

Add both single and multiple quotes testing with renaming. Fix for Windows EOL `\r\n` (thanks to @smrq in olov#56).

frapontillo · 2014-07-28T11:37:53Z

Tests written and passed.
I have integrated e57d008 by @smrq by cherry picking.

h2non · 2014-07-29T10:16:51Z

Waiting for the merge!

olov · 2014-08-02T11:46:19Z

Ok, can you fix the following:

rebase against HEAD
the rename-map should be a https://www.npmjs.org/package/stringmap, not a JS object
the ng-annotate API should take a rename-map in the form of [{from: string, to: string}, ..], then convert it to stringmap
i don't want to manually keep the test input and output files in sync with the main original.js file so for that reason you can slim down the two files into something that tests renaming with less examples.

Thanks!

frapontillo · 2014-08-03T15:57:28Z

Sure, I'm on vacation right now, I will be able to get back to it after
14th August.
Il 02/ago/2014 13:46 "Olov Lassus" [email protected] ha scritto:

Ok, can you fix the following:

rebase against HEAD

the rename-map should be a https://www.npmjs.org/package/stringmap,
not a JS object

the ng-annotate API should take a rename-map in the form of [{from:
string, to: string}, ..], then convert it to stringmap

i don't want to manually keep the test input and output files in
sync with the main original.js file so for that reason you can slim down
the two files into something that tests renaming with less examples.

Thanks!

—
Reply to this email directly or view it on GitHub
#47 (comment).

Conflicts: ng-annotate-main.js ng-annotate.js nginject-comments.js run-tests.js

frapontillo · 2014-08-18T07:27:18Z

About the rename API, could you please confirm that ng-annotate.js should take a JSON representation in the form of "[{\"from\":\"string\", \"to\":\"newString\"}]", then pass it to ng-annotate-main.js as a stringmap?

CLI example:

$ node --harmony ng-annotate.js -a --rename "[{\"from\":\"$a\", \"to\":\"$aRenamed\"}]" tests/rename.js

olov · 2014-08-18T07:36:46Z

No, the CLI can function as you already made it, the API however: require("ng-annotate")(src, {add: true, rename: [{from: "hello", to: "kitty"}, {from: "oi", to: "yo"}]})

frapontillo · 2014-08-18T08:14:35Z

So every from-to association should be transformed into a stringmap?

olov · 2014-08-18T08:17:39Z

const renamemap = stringmap();
rename.forEach(function(pair) { renamemap.set(pair.from, pair.to) })

The rename CLI option remains the same, but it is passed to `ng-annotate-main.js` as a from-to object collection; it is then transformed and used as a stringmap. Rename tests have been separated from the original full test and simplified to test against the main cases.

frapontillo · 2014-08-18T08:56:51Z

Changes have been pushed, thank you for your support in this :)

mgcrea · 2014-09-15T08:59:17Z

@olov Would be great if you could merge this, it's blocking our release of a compat build for angular-strap. Thanks!

frapontillo reviewed Jul 16, 2014
View reviewed changes

Use let instead of var, fix build

2c189c4

frapontillo added 2 commits July 17, 2014 13:02

Use let instead of var, fix build

b330a25

Merge branch 'rename' of github.com:frapontillo/ng-annotate into rename

e1efabd

olov added the enhancement label Jul 17, 2014

mgol reviewed Jul 17, 2014
View reviewed changes

frapontillo mentioned this pull request Jul 20, 2014

Compatibility with UI Bootstrap mgcrea/angular-strap#521

Closed

olov added the pr-in-progress label Jul 27, 2014

frapontillo and others added 3 commits July 28, 2014 09:36

Merge remote-tracking branch 'upstream/master' into rename

b6f2940

Add rename tests, transform remaining let to const

98a6a0a

Add rename tests, fix test on Windows

e29ef7a

Add both single and multiple quotes testing with renaming. Fix for Windows EOL `\r\n` (thanks to @smrq in olov#56).

Merge remote-tracking branch 'upstream/master' into rename

8bed6d1

Conflicts: ng-annotate-main.js ng-annotate.js nginject-comments.js run-tests.js

fix: rename provider names

a77e91c

olov merged commit a77e91c into olov:master Sep 15, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --rename option to rename declarations and references #47

add --rename option to rename declarations and references #47

frapontillo commented Jul 16, 2014

frapontillo Jul 16, 2014

olov commented Jul 16, 2014

frapontillo commented Jul 16, 2014

frapontillo commented Jul 17, 2014

mgol Jul 17, 2014

mgcrea commented Jul 27, 2014

olov commented Jul 27, 2014

frapontillo commented Jul 27, 2014

frapontillo commented Jul 28, 2014

h2non commented Jul 29, 2014

olov commented Aug 2, 2014

frapontillo commented Aug 3, 2014

frapontillo commented Aug 18, 2014

olov commented Aug 18, 2014

frapontillo commented Aug 18, 2014

olov commented Aug 18, 2014

frapontillo commented Aug 18, 2014

mgcrea commented Sep 15, 2014

add --rename option to rename declarations and references #47

add --rename option to rename declarations and references #47

Conversation

frapontillo commented Jul 16, 2014

frapontillo Jul 16, 2014

Choose a reason for hiding this comment

olov commented Jul 16, 2014

frapontillo commented Jul 16, 2014

frapontillo commented Jul 17, 2014

mgol Jul 17, 2014

Choose a reason for hiding this comment

mgcrea commented Jul 27, 2014

olov commented Jul 27, 2014

frapontillo commented Jul 27, 2014

frapontillo commented Jul 28, 2014

h2non commented Jul 29, 2014

olov commented Aug 2, 2014

frapontillo commented Aug 3, 2014

frapontillo commented Aug 18, 2014

olov commented Aug 18, 2014

frapontillo commented Aug 18, 2014

olov commented Aug 18, 2014

frapontillo commented Aug 18, 2014

mgcrea commented Sep 15, 2014