Further non-breaking cleanup & improvements #20

raphinesse · 2018-06-17T22:25:26Z

See commits for details

brody4hire

Looks nice in general. I pushed some suggested changes:

suggested squash fixes to the main function:
- do not reassign function arguments - looks ugly and is a bad idea according to: https://spin.atomicobject.com/2011/04/10/javascript-don-t-reassign-your-function-arguments/
- split up cordovaCreate function to deal with dest dir in a nicer way:
  - favors const over let
  - ugly TODO item seems to go away
test fixes:
- mark NPM package test for specific version
- restore & fix NPM package test with no specific version (it would be ideal if you would fix rather than remove the test in f4d455c)

And a few more items:

Shouldn't README be updated to reflect the API options?
New API option does not seem to be directly covered by spec, would like to see at least 1 test cover the new API option.

brody4hire · 2018-07-05T01:43:52Z

index.js

-    return Promise.resolve().then(function () {
-        events = setupEvents(extEvents);
-        events.emit('verbose', 'Using detached cordova-create');
+* Delegates to the correct function based on the given arguments


Should the exported API options be better documented here?

Also shouldn't README.md be updated to reflect the new API options, along with the deprecation notice?

brody4hire · 2018-07-05T01:46:16Z

index.js

+};
+
+/**
+* Legacy interface. See README for documentation


~~Shouldn't there be a deprecation notice here and in README.md?~~

The deprecation notice is not needed at this point since there is no longer a change to the external interface in this PR.

brody4hire · 2018-07-05T01:46:48Z

index.js

+* Legacy interface. See README for documentation
+*/
+function cordovaCreateLegacyAdapter (dir, id, name, cfg, extEvents) {
+    // TODO DEPRECATION NOTICE


Shouldn't we do this now as requested in other comments?

brody4hire · 2018-07-05T06:01:58Z

I just pushed a few more changes, would be happy to move them to another PR:

do not overwrite events object
use forEach instead of for loop
use const whenever possible

raphinesse · 2018-07-05T09:31:58Z

Thanks for your extensive review @brodybits!

I just pushed a few more changes, would be happy to move them to another PR

I'd like to avoid any additional changes to prevent conflicts with my existing work for apache/cordova-discuss#89. Unfortunately all your suggested additional changes actually do conflict with that.

My plan is as follows

Reach a consensus on which parts of Breaking changes in cordova-create cordova-discuss#89 we do want
Adapt my existing work and get it into master (Including documentation and tests)
Do another round of cleanup. Including ES20XXify and all.

So I'd suggest you do not yet create another cleanup PR.

I will squash the test commit you provided, thanks.

Regarding the supposed new interface

I've been a bit overzealous here. I will remove the delegation function and add that as part of a PR for apache/cordova-discuss#99. The interface introduced here is not as described in apache/cordova-discuss#99 and should be considered private.

I have mixed feelings regarding your proposed changes for the main function. I will elaborate in code comments.

raphinesse · 2018-07-05T10:45:38Z

@brodybits Regarding the suggested changes to the main function:

do not reassign function arguments - looks ugly and is a bad idea according to: https://spin.atomicobject.com/2011/04/10/javascript-don-t-reassign-your-function-arguments/

Even though I don't particularly like to assign to arguments, I like the alternatives here less. Especially because of conflicts with existing work and because I hope for this to be going away with future refactors anyway (see below).

Furthermore, I don't think the dangers outlined in the post above apply to us. We do not use arguments at all and never should do so in the future. In ES6+ code we can always use rest parameters instead. Moreover, we are not mixing up args as the author does, but refine the exisiting arguments while not changing their meaning.

split up cordovaCreate function to deal with dest dir in a nicer way:

This change breaks the Single Level of Abstraction principle (example).

After landing any major breaking changes I do want to split the main function up, though. But in a way that actually explains what's happening and improves unit-testability. Something like:

normalizeArguments()
resolveTemplate()
expandTemplate()

Do you think you can live with my updated changes given my reasoning and plans outlined above?

If this lands I would prepare a preview PR for the work I've done on the breaking changes next.

Including: - Unwrap deeply nested options object - Rename options object to `opts` - Shallow copy `opts` to avoid mutating caller's object - Make Q-wrapping explicit Co-authored-by: Christopher J. Brody <[email protected]>

This reduces pollution of user's home directory and definitely makes the previously performed makeshift cache-busting obsolete since we fetch into a fresh temp directory every time. Addresses §6 of apache/cordova-discuss#89 Co-authored-by: Christopher J. Brody <[email protected]>

This does _not_ change the tests itself, even though they are broken

The errors from fetch were strongly emphasized (all red) for no obvious reason. Moreover, everything after the first error event emission never ran since the error event seems to cause an error to be thrown.

brody4hire · 2018-07-05T12:51:22Z

Thanks @raphinesse for going through my comments. I think the most important thing was not to break any existing tests. While I would like to see the other points addressed sometime I think they are minor enough to leave for later. +1 for cleaning up the internal interface while waiting for agreement on the external interface.

A couple side points:

I wish this PR had come with a reminder of apache/cordova-discuss#89 for the context. (I totally forgot about the discussion, didn't even remember commenting on it until I took another look.) But better late than never:)

I was surprised that you named me as co-author of e020d8d, don't think I had anything to do with the changes there.

raphinesse · 2018-07-05T13:45:48Z

I wish this PR had come with a reminder of apache/cordova-discuss#89 for the context.

Yes, I thought so too when writing my replies. Sorry for that. My bad 😞

I was surprised that you named me as co-author of e020d8d, don't think I had anything to do with the changes there.

I incorporated your changes regarding arguments assignment into the adapter function. Should I remove you?

brody4hire · 2018-07-05T14:01:39Z

I incorporated your changes regarding arguments assignment into the adapter function.

Missed that one (still don't see it). Thanks for the credit, though.

raphinesse force-pushed the cleanup-2 branch from 45cd57c to 759ccdd Compare June 17, 2018 22:26

raphinesse force-pushed the cleanup-2 branch 2 times, most recently from e18ff3c to 70cfb17 Compare July 4, 2018 22:05

raphinesse requested review from dpogue and brody4hire July 4, 2018 22:08

brody4hire suggested changes Jul 5, 2018

View reviewed changes

raphinesse force-pushed the cleanup-2 branch from 105280f to 7055c85 Compare July 5, 2018 10:48

raphinesse and others added 5 commits July 5, 2018 12:48

Simplify main function behind legacy adapter

e020d8d

Including: - Unwrap deeply nested options object - Rename options object to `opts` - Shallow copy `opts` to avoid mutating caller's object - Make Q-wrapping explicit Co-authored-by: Christopher J. Brody <[email protected]>

Use path-is-inside to check for invalid nesting

169e0d7

Cleaner check for need of cordova-fetch

c063c87

This does _not_ change the tests itself, even though they are broken

Do not wrap errors from fetch & remove dead code

227ab66

The errors from fetch were strongly emphasized (all red) for no obvious reason. Moreover, everything after the first error event emission never ran since the error event seems to cause an error to be thrown.

raphinesse force-pushed the cleanup-2 branch from 7055c85 to 227ab66 Compare July 5, 2018 10:48

brody4hire approved these changes Jul 5, 2018

View reviewed changes

raphinesse merged commit 86b0585 into apache:master Jul 5, 2018

raphinesse deleted the cleanup-2 branch July 5, 2018 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further non-breaking cleanup & improvements #20

Further non-breaking cleanup & improvements #20

raphinesse commented Jun 17, 2018

brody4hire left a comment

brody4hire Jul 5, 2018

brody4hire Jul 5, 2018 •

edited

Loading

brody4hire Jul 5, 2018

brody4hire commented Jul 5, 2018

raphinesse commented Jul 5, 2018

raphinesse commented Jul 5, 2018

brody4hire commented Jul 5, 2018

raphinesse commented Jul 5, 2018

brody4hire commented Jul 5, 2018

Further non-breaking cleanup & improvements #20

Further non-breaking cleanup & improvements #20

Conversation

raphinesse commented Jun 17, 2018

brody4hire left a comment

Choose a reason for hiding this comment

brody4hire Jul 5, 2018

Choose a reason for hiding this comment

brody4hire Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

brody4hire Jul 5, 2018

Choose a reason for hiding this comment

brody4hire commented Jul 5, 2018

raphinesse commented Jul 5, 2018

Regarding the supposed new interface

raphinesse commented Jul 5, 2018

brody4hire commented Jul 5, 2018

raphinesse commented Jul 5, 2018

brody4hire commented Jul 5, 2018

brody4hire Jul 5, 2018 •

edited

Loading