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 to remove leading dashes and underscores #74

Open
wants to merge 1 commit into
base: master
from

Conversation

@jathak
Copy link
Member

commented Aug 13, 2019

This partially resolves (but not fully) #48, allowing members that start with a dash or an underscore to be treated as library-private instead of module-private, but it's an all-or-nothing flag instead of detecting which members are used outside of the module they're declared in.

Add flag to remove leading dashes and underscores
This partially resolves #48, allowing members that start with a dash or
an underscore to be treated as library-private instead of
module-private, but it's an all-or-nothing flag instead of detecting
which members are used outside of the module they're declared in.

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

@nex3

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

The more I think about this behavior, the more I don't like the way it does an unambiguously incorrect thing by default when a _-prefixed member is used across files (it produces $module._name). I think it's important for the default behavior, without any flags, to produce something at least functional if not ideal.

I think the ideal behavior here would probably be to do an initial non-modifying pass that resolves each member reference to the definition it refers to, and stores a reverse pointer as well. Then when you do a second modifying pass, you'd have the information available to determine whether there are any uses of a _-prefixed member outside of its module, and migrate it accordingly.

That represents a pretty big change, though, so I think it's also reasonable to just make the default be "remove all _ prefixes" and have a flag (--private-members?) that opts in to preserving them. Because the user is opting into that flag, you can then throw an error if there are any cases that would end up producing something like $module._name.

@jathak

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Yeah, that makes sense. I started adding a visitor for the first pass in a separate branch and it was less complicated than I thought it would be, so I'll finish up that when I get back from vacation and then use that for a more complete fix of #48.

The first pass visitor could probably also be useful outside of the module migrator if we wanted to make a refactoring tool for things like member renaming (not sure if that's something that already exists for Sass)

@nex3

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

There's basically never been any static analysis of any kind for Sass, so you're treading new ground here 😃. This is the sort of thing we'll want to write a Kythe plugin as well.

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.