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

i18n: move YARD::CLI::I18n::* to YARD::I18n::* #527

Merged
merged 3 commits into from
Apr 30, 2012

Conversation

kou
Copy link
Contributor

@kou kou commented Apr 30, 2012

Those changes are corresponding to 1. and 3. in #395:

#395 (comment)

  1. I'd like it if you could move your CLI::I18n subclasses into a
    YARD::I18n namespace. I'm not a fan of classes inside of classes,
    organizationally speaking, and it's hard to promote component reuse
    for these classes if they're inside of a CLI namespace. Presumably
    someone might want to use the PotGenerator class directly, for
    instance, and they shouldn't have to go through the CLI. So, I've
    marked those classes as @private for now so that nobody uses them
    from that namespace. Once you move them into a more appropriate
    location, you can remove the @private declaration and we can support
    them as a public API.
  2. ...
  3. Ditto for moving the tests for the inner classes like
    PotGenerator and Test into their own files. I'm also not a fan of
    multiple classes in a single _spec.rb file.

kou added 3 commits April 30, 2012 23:33
This commit is corresponding to 1. and 3. in lsegal#395:

  lsegal#395 (comment)

  1. I'd like it if you could move your CLI::I18n subclasses into a
  YARD::I18n namespace. I'm not a fan of classes inside of classes,
  organizationally speaking, and it's hard to promote component reuse
  for these classes if they're inside of a CLI namespace. Presumably
  someone might want to use the PotGenerator class directly, for
  instance, and they shouldn't have to go through the CLI. So, I've
  marked those classes as @Private for now so that nobody uses them
  from that namespace. Once you move them into a more appropriate
  location, you can remove the @Private declaration and we can support
  them as a public API.

  3. Ditto for moving the tests for the inner classes like
  PotGenerator and Test into their own files. I'm also not a fan of
  multiple classes in a single _spec.rb file.
@lsegal lsegal merged commit 1f2f2c0 into lsegal:master Apr 30, 2012
@lsegal
Copy link
Owner

lsegal commented Apr 30, 2012

Thanks! We landed this just in time for 0.8.0.

One thing I noticed after you documented the code is that PotGenerator#messages has a very long type signature. To me, this usually implies that the API is equally complex to use. The type: Hash{:locations => Array<Array[String, Integer]>, :comments => Array<String>} to me screams of refactoring into a separate class with .locations and .comments attributes. That way we can have a properly documented Message (would this be the right name? or PotMessage?) class component that has meaning in our system, and #messages would turn into Hash{String=>Message}, which is much clearer to me. Can you do this refactor?

No rush on this one, I'm not expecting it for 0.8.0, since we are releasing today, but we can target 0.8.1. I've marked the #messages API method as "private" (with @api private) for now to notify users that we are going to change this slightly.

@kou
Copy link
Contributor Author

kou commented Apr 30, 2012

Thanks for merging!
It's great news that 0.8.0 ships a I18N feature!

I'll do the refactor after 0.8.0 is released.

kou added a commit to kou/yard that referenced this pull request May 2, 2012
This commit is corresponding to comment in lsegal#527:

  lsegal#527 (comment)

  One thing I noticed after you documented the code is that
  PotGenerator#messages has a very long type signature. To me, this
  usually implies that the API is equally complex to use. The type:
  Hash{:locations => Array<Array[String, Integer]>, :comments =>
  Array<String>} to me screams of refactoring into a separate class with
  .locations and .comments attributes. That way we can have a properly
  documented Message (would this be the right name? or PotMessage?)
  class component that has meaning in our system, and #messages would
  turn into Hash{String=>Message}, which is much clearer to me. Can you
  do this refactor?
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