Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag for forwarding members from entrypoint #72

Merged
merged 3 commits into from Aug 2, 2019

Conversation

@jathak
Copy link
Member

commented Aug 1, 2019

Resolves #47.

Adds a new flag --forward which can be none (default), all, or prefixed.

If --forward=all, all global members from dependencies will be forwarded from the entrypoint.

If --forward=prefixed, only members that were previously prefixed with the value of --remove-prefix will be forwarded.

Add flag for forwarding members from entrypoint
Resolves #47.

Adds a new flag `--forward` which can be `none` (default), `all`, or
`prefixed`.

If `--forward=all`, all global members from dependencies will be
forwarded from the entrypoint.

If `--forward=prefixed`, only members that were previously prefixed with
the value of `--remove-prefix` will be forwarded.

@jathak jathak requested a review from nex3 Aug 1, 2019

@nex3
Copy link
Contributor

left a comment

Please add a spec for --forward=all with an imported library that doesn't define any members.

if (argResults['forward'] == 'prefixed' &&
argResults['remove-prefix'] == null) {
throw MigrationException(
'Error: Must provide --remove-prefix when --forward=prefixed');

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor

I might make this a little clearer about why this is necessary. Maybe "You must provide --remove-prefix with --forward=prefixed so we know which prefixed members to forward" or something like that?

Also, I'd avoid putting "Error: " at the beginning of literal exception messages. If you want a header like that, it should be the responsibility of the error handler that catches and prints the exceptions.

@@ -96,12 +114,15 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// should be removed.
final String prefixToRemove;

/// The value of the --forward flag, either 'none', 'all', or 'prefixed'.
final String forwardOption;

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor

"Option" here is kind of redundant. I'd probably just call it forward.

Also, I'd translate the string value to an enum at some point before it gets here. It's more overhead, but it gives you type-checking for any checks you do in the body of this visitor.

return _getEntrypointForwards() + uses.join() + results;
}

/// Returns whether an member of [name] should be forwarded in the entrypoint.

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor
Suggested change
/// Returns whether an member of [name] should be forwarded in the entrypoint.
/// Returns whether the member named [name] should be forwarded in the entrypoint.

// When not all members from a dependency should be forwarded, determine
// whether to use a `show` clause or a `hide` clause based on the number
// of members in each set.

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor

Nice! I like how this avoids show/hide declarations when they aren't necessary.

// whether to use a `show` clause or a `hide` clause based on the number
// of members in each set.
if (hiddenCount > 0) {
if (shownCount <= hiddenCount) {

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor

This is clever, but I think we probably want to decide which style we want to encourage and use that universally. I lean towards defaulting to hide.

_additionalUseRules.add(simplePath);
_namespaces[node.span.sourceUrl] = namespaceForPath(simplePath);
}
return _namespaces[node.span.sourceUrl];
}

/// Converts an absolute URI for a stylesheet into the simplest string that

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor
Suggested change
/// Converts an absolute URI for a stylesheet into the simplest string that
/// Converts an absolute URL for a stylesheet into the simplest string that

Also in the function name. This is kind of a personal crusade for me, but according to the whatwg spec "URL" should be universally preferred over "URI" (contrary to Dart's unfortunate choice of class name).

/// could be used to depend on that stylesheet from the current one in a use,
/// forward, or import rule.
String _absoluteUriToDependency(Uri uri) {
var relativePath = p.relative(uri.path, from: p.dirname(_currentUrl.path));

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor
Suggested change
var relativePath = p.relative(uri.path, from: p.dirname(_currentUrl.path));
var relativePath = p.url.relative(uri.path, from: p.url.dirname(_currentUrl.path));

When you're working with URLs, you always want to use p.url. Otherwise you're interpreting URLs as native paths on whatever the current OS is, which will lead to all sorts of nasty edge-cases. Same throughout this function.

@jathak jathak requested a review from nex3 Aug 2, 2019

@nex3

nex3 approved these changes Aug 2, 2019

@@ -12,6 +12,7 @@ import 'package:args/args.dart';
import 'package:sass/src/ast/sass.dart';

import 'package:path/path.dart' as p;
import 'package:sass_migrator/src/migrators/module/forward_type.dart';

This comment has been minimized.

Copy link
@nex3

nex3 Aug 2, 2019

Contributor

It's generally preferred to use relative imports for files within the same lib/. Also below.

@jathak jathak merged commit f5575ea into master Aug 2, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jathak jathak deleted the entrypoint-forward branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.