Skip to content

Fix #3502: Define param variables when expansion #3791

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

Merged
merged 1 commit into from
Jan 14, 2015

Conversation

lydell
Copy link
Collaborator

@lydell lydell commented Jan 13, 2015

No description provided.

for {name: p} in @params when param not instanceof Expansion
if p.value then o.scope.add p.value, 'var', yes
for p in @params when p not instanceof Expansion
if p.name.value then o.scope.add p.name.value, 'var', yes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems weird to have a when and a single if as body

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for p in @params
  if p not instanceof Expansion and p.name.value
    o.scope.add p.name.value, 'var', yes

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps:

for p in @params
  o.scope.add p.name.value, 'var', yes if p.name?.value

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go for the first (or use some kind of any)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already pushed a new version before you commented ...

michaelficarra added a commit that referenced this pull request Jan 14, 2015
Fix #3502: Define param variables when expansion
@michaelficarra michaelficarra merged commit 669e7fe into jashkenas:master Jan 14, 2015
@lydell lydell deleted the issue-3502 branch January 14, 2015 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants