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

fixed issue in translating nested hash with multiple string keys #518

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jitendra1998
Copy link
Contributor

@jitendra1998 jitendra1998 commented Apr 5, 2020

Fixed issue #514 in this pr

@dangerpr-bot
Copy link

dangerpr-bot commented Apr 5, 2020

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [#518](https://github.com/hashie/hashie/pull/518): fixed issue in translating nested hash with multiple string keys - [@jitendra1998](https://github.com/jitendra1998).

Generated by 🚫 Danger

@dblock
Copy link
Member

dblock commented Apr 5, 2020

Awesome.

  • Add CHANGELOG please.
  • Is this going to work recursively, add a test with one more nested level of property.
  • There's some kind of problem in an integration test that looks unrelated, maybe try to fix it in a separate PR? We'll have to hold merging changes until a green build.

@jitendra1998
Copy link
Contributor Author

Awesome.

  • Add CHANGELOG please.
  • Is this going to work recursively, add a test with one more nested level of property.
  • There's some kind of problem in an integration test that looks unrelated, maybe try to fix it in a separate PR? We'll have to hold merging changes until a green build.

sure will create a new pr for fixing failed integration spec

@dblock
Copy link
Member

dblock commented Apr 12, 2020

Thanks for #520. Rebase?

@@ -28,7 +28,7 @@ scheme are considered to be bugs.

### Fixed

* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Put this back ;)

@@ -28,7 +28,7 @@ scheme are considered to be bugs.

### Fixed

* Your contribution here.
* [#518](https://github.com/hashie/hashie/pull/518): fixed issue in translating nested hash with multiple string keys - [@jitendra1998](https://github.com/jitendra1998).
Copy link
Member

Choose a reason for hiding this comment

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

Needs to start with a capital Fixed.

name = name.to_s
!!properties.find { |property| property.to_s == name }
if name.is_a? ::Array
name.all? { |att| !!properties.find { |property| property.to_s == att.to_s } }
Copy link
Member

Choose a reason for hiding this comment

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

Does name require a translation again in the Array case? Should come up with a nested test case with translations at all levels.

If so, then name.all? { |att | property?(att) }?

@dblock
Copy link
Member

dblock commented Oct 3, 2020

Want to try and finish this @jitendra1998 ?

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.

None yet

3 participants