Skip to content

Added support for compiling more deeply nested namespaces#132

Closed
hadrianoliver wants to merge 1 commit intomessageformat:masterfrom
hadrianoliver:recursive_namespaces
Closed

Added support for compiling more deeply nested namespaces#132
hadrianoliver wants to merge 1 commit intomessageformat:masterfrom
hadrianoliver:recursive_namespaces

Conversation

@hadrianoliver
Copy link
Contributor

@hadrianoliver hadrianoliver commented Feb 10, 2016

I'm working on an system that uses more levels of namespaces than are currently supported by the compile function. This change adds support for any arbitrary depth of namespace nesting by using a recursive function to handle them instead of the current nested for loops.

Example:

var messages = {
  namespace1: {
    namespace2: {
      message: "Compile can't handle this without this change!"
    }
  }
}

var MessageFormat = require('messageformat');
var messageformat = new MessageFormat();
var compiled = messageformat.compile(messages);

Results in the error:

Warning: Parser error with namespace2 in namespace1: TypeError: Object # has no method 'charCodeAt' Use --force to continue.

@eemeli
Copy link
Member

eemeli commented Feb 10, 2016

This looks pretty good. If you could add a test case for the new functionality, I'll accept the PR.

@hadrianoliver
Copy link
Contributor Author

hadrianoliver commented Feb 11, 2016

On reflection I don't believe this change would be good, even though it works as I had intended. I'd rather change the system I'm working on to behave in a more 'usual' manner and make the behaviour of MessageFormat::compile() consistent with the output from the build-time compilation. When I get this working in a way I'm happy with I'll make another pull request with tests.

@eemeli eemeli mentioned this pull request Feb 24, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hadrianoliver @eemeli