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

alias support - fixes #30 #31

Merged
merged 4 commits into from
Jan 5, 2017
Merged

alias support - fixes #30 #31

merged 4 commits into from
Jan 5, 2017

Conversation

StephanHoyer
Copy link
Owner

No description provided.

@StephanHoyer StephanHoyer changed the title Enh 30 alias support #30 alias support Jan 4, 2017
@StephanHoyer StephanHoyer changed the title #30 alias support alias support - fixes #30 Jan 4, 2017
@StephanHoyer
Copy link
Owner Author

@maranomynet plz review an merge if you are ok with it.

I'll do a release then.

@maranomynet
Copy link
Collaborator

Should the order of keys in the translations object matter?

i.e. should this test pass?

it('should be agnostic to the order of key declarations', function () {
  expect(translate.resolveAliases({
    C: '< {{B}} >',
    B: 'foo {{A}} bar',
    A: 'bar',
  })).to.eql(translate.resolveAliases({
    A: 'bar',
    B: 'foo {{A}} bar',
    C: '< {{B}} >',
  }))
})

Supporting this would require a slightly more complicated resolution algorithm, but not that much.

@maranomynet
Copy link
Collaborator

It seems that the circular reference detection isn't only detecting circular references.

@maranomynet
Copy link
Collaborator

Also, I would have expected this test to pass:

it('should allow multiple aliases per string', function () {
  expect(translate.resolveAliases({
    A: 'bar',
    B: 'foo {{A}} {{A}}',
    C: 'foo {{B}} {{A}}'
  })).to.eql({
    A: 'bar',
    B: 'foo bar bar',
    B: 'foo foo bar bar bar'
  })
})

@maranomynet
Copy link
Collaborator

maranomynet commented Jan 5, 2017

I pushed the failing tests just in case... feel free to revert aaebaad if you think either of these is unreasonable or unnecessary atm.

@StephanHoyer
Copy link
Owner Author

Thanks.

Just tried to fix this... not trivial.

@StephanHoyer
Copy link
Owner Author

ok, just found a simple solution. Feel free to check again

@maranomynet maranomynet merged commit eb9207c into master Jan 5, 2017
@maranomynet
Copy link
Collaborator

Looks great. I can't think of any other missing features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants