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

fix(crush): fix handling of period-containing property names #95

Merged
merged 7 commits into from
Jul 20, 2024

Conversation

stefaanv
Copy link
Contributor

@stefaanv stefaanv commented Jul 8, 2024

Resolves sodiray/radash#365
Resolves #23

@stefaanv stefaanv requested a review from aleclarson as a code owner July 8, 2024 19:15
@stefaanMLB
Copy link

I have solved the "dot in property name" problem and added some more tests for the crush function.
What's next ? I'm not very fluent with PR's. I hope I did everything right ?

@aleclarson aleclarson force-pushed the main branch 2 times, most recently from 2154f96 to 6a4b4f6 Compare July 12, 2024 00:23
Comment on lines 53 to 58
const obj = {
'a.b': { 'c.d': 123.4 }
}
expect(_.crush(obj)).toEqual({
'a.b.c.d': 123.4
})
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't preserve the intent though. We'll probably need support for "a.b"."c.d" paths.

Choose a reason for hiding this comment

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

@aleclarson I don't understand your remark.
The test is just a slightly more elaborate example of key names with a dot inside.
How does this not preserve intent ? It's just a test, it passes.
What do you mean with "a.b"."c.d" paths ? AFAIK it's not possible to specify such a path in javascript.

Copy link
Member

@aleclarson aleclarson Jul 13, 2024

Choose a reason for hiding this comment

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

Sorry for the vagueness. We can still merge this PR, since you fixed crush to pass the failed test case.

But in the future, we should avoid crushing ["a.b"]["c.d"] to a.b.c.d since that won't reproduce the same object when used with construct. Example below:

import { crush, construct } from 'radashi'

test('crush is the opposite of construct', () => {
  const source = { 'a.b': { 'c.d': 1 } }

  // This currently fails:
  expect(construct(crush(source))).toEqual(source)
})

So we can merge this PR now or, if you'd like, you could also fix the test case above and include that fix in this PR. Either way works, so just let me know!

Choose a reason for hiding this comment

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

There may be purposes for crush() other than being the inverse of construct().
Probably a get() will be a good alternative for that in most cases, but the current crush() implementation is flawed as it returns undefined when there's dots in the propserty names..
I have made a new commit where the inner recursive function is outside of the crush() function, That should be slightly more efficient in term of closures.

Copy link
Member

Choose a reason for hiding this comment

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

True. Okay, awaiting your new commit. 👍

Choose a reason for hiding this comment

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

@aleclarson
I am unable to commit to my own PR ?
Get this error messages. Any idea what might be going on ?

$ git push
remote: Permission to radashi-org/radashi.git denied to stefaanv.
fatal: unable to access 'https://github.com/radashi-org/radashi.git/': The requested URL returned error: 403

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what your git remote -v looks like, but seems like you need to set the branch's upstream with:

git remote set-url origin https://github.com/yourusername/radashi.git
git push -u origin your-branch-name

Replace 'yourusername' with your GitHub username and 'your-branch-name' with your actual branch name. This will update your origin to point to your fork and then push your branch there.

After this, you can just use git push for future pushes on this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aleclarson thanks for the help, I succeeded in committing this changes.
You can merge the PR

Copy link

@stefaanMLB stefaanMLB left a comment

Choose a reason for hiding this comment

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

OK

@aleclarson aleclarson force-pushed the fix/crush-dot-property branch from 0a7bfb9 to b7370c7 Compare July 19, 2024 17:47
Copy link
Member

@aleclarson aleclarson left a comment

Choose a reason for hiding this comment

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

Thank you for spending time on this!

I've reworked it, but the general solution you implemented remains intact. I added a failing test case, which I then fixed, that will ensure support for built-in object types like Map, Set, etc (these will not be crushed).

I also took time to improve the return type. If a crushed object has nested objects, we cannot assume anything about the value types, because of TypeScript limitations. In particular, we cannot know which object types will pass the isObject check (and therefore get crushed). There are “halfway solutions” to this, like keeping a union type of all the built-in object types, but I opted for the safer approach of “assume nothing”.

Let me know what you think! And thanks again.

@aleclarson aleclarson changed the title fixes support dot in property name #23 fix(crush): fix handling of period-containing property names Jul 19, 2024
@aleclarson
Copy link
Member

aleclarson commented Jul 19, 2024

Oh btw, I undid your change in b3eef45, as hoisting functions is a micro-optimization that Radash (and by extension, Radashi) hasn't done in the past. If you have a strong opinion on this, we can discuss over in the discussions area. 👍

@stefaanv
Copy link
Contributor Author

@aleclarson
thank for all this. That's some impressive typescript code you've written !
I'm still learning, this is a lot of fun.
Anything I need to do now ? I see that the code is not in the main branch yet.

@stefaanv stefaanv closed this Jul 20, 2024
@stefaanv
Copy link
Contributor Author

Sorry, I guess I've pushed the close button too soon

@stefaanv stefaanv reopened this Jul 20, 2024
@aleclarson aleclarson merged commit a5e5180 into radashi-org:main Jul 20, 2024
14 of 16 checks passed
@aleclarson
Copy link
Member

All merged now. Just wanted to wait in case you had any objections :)

@stefaanv stefaanv deleted the fix/crush-dot-property branch July 20, 2024 16:39
Copy link

A new beta version 12.2.0-beta.313bfb4 has been published to NPM. 🚀

To install:

The radashi@beta tag also includes this PR.

See the changes

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.

crush() doesn't handle dots in property names well
3 participants