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

Basic support for namespacing get-function calls #13

Merged
merged 5 commits into from Mar 13, 2019

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Contributor

jathak commented Mar 9, 2019

Resolves #11.

If the argument passed to get-function is a string literal, we'll add a namespace if necessary.

For all other get-function calls, we'll emit a warning saying that a namespace may be required. We might eventually be able to detect some of these with some more sophisticated logic here, but for now we'll just handle the basics.

jathak added some commits Mar 9, 2019

Add basic support for namespacing get-function
Resolves #11.

If the argument passed to get-function is a string literal, we'll add a
namespace if necessary.

For all other get-function calls, we'll emit a warning saying that a
namespace may be required. We might eventually be able to detect some of
these with some more sophisticated logic here, but for now we'll just
handle the basics.

@jathak jathak requested a review from nex3 Mar 9, 2019

} else {
_warn(
"WARNING: Could not determine if namespace is required for "
"get-function call @",

This comment has been minimized.

@nex3

nex3 Mar 9, 2019

Nit: What's this @ for?

Show resolved Hide resolved lib/src/migrator.dart Outdated
Show resolved Hide resolved lib/src/migrator.dart Outdated
Show resolved Hide resolved test/migrations/namespace_get_function.hrx Outdated
Use correct syntax for get-function migration
Also changes warning message to use span.message and updates tests to
use only ASCII characters for checking log.

@jathak jathak requested a review from nex3 Mar 11, 2019

var beforeParen = node.span.end.offset - 1;
_currentMigration.patches.add(Patch(
node.span.file.span(beforeParen, beforeParen),
", \$module: $namespace"));

This comment has been minimized.

@nex3

nex3 Mar 11, 2019

Nit: Emit quotes around the namespace, in case it's a color name or something. There is actually a Sass library named true, so this isn't even purely hypothetical.

,
8 | $fn4: get-function($x);
| ^^
'

This comment has been minimized.

@nex3

nex3 Mar 11, 2019

Also test a getting a function from a core library when the function's namespaced name doesn't match its original name, and getting a function from a library named true or whatever.

@jathak jathak requested a review from nex3 Mar 11, 2019

@nex3

nex3 approved these changes Mar 11, 2019

if (nameArgument is! StringExpression ||
(nameArgument as StringExpression).text.asPlain == null) {
print(nameArgument.span.message(
"WARNING - get-function call may require \$module parameter"));

This comment has been minimized.

@nex3

nex3 Mar 11, 2019

It's probably a good idea to print this to stderr.

This comment has been minimized.

@jathak

jathak Mar 11, 2019

Author Contributor

Is there an equivalent to the prints matcher for testing stderr?

This comment has been minimized.

@nex3

nex3 Mar 12, 2019

Unfortunately no there isn't. Usually when I'm testing command-line executables, I just run them via Process.runSync() or the test_process package so I don't have to worry about mocking out all the APIs.

This comment has been minimized.

@jathak

jathak Mar 13, 2019

Author Contributor

Okay, in that case, I'll leave this as is for now. I've made an issue (#14) for this, which I'll handle in a separate PR

Show resolved Hide resolved test/migrations/namespace_get_function.hrx

@jathak jathak merged commit 502aace into master Mar 13, 2019

@jathak jathak deleted the get-function branch Mar 13, 2019

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