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 compact type for objects #1611

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

Conversation

mathieutu
Copy link
Contributor

@mathieutu mathieutu commented Sep 30, 2020

Hi,
For now when you pass an object to compact function there is a type problem event if everything is good.

This PR fix this behaviour.

This PR fix another issue with compact, so I didn't do anything which could collide with it.
#1603

Thanks for the job.

@char0n char0n self-requested a review October 4, 2020 16:16
Copy link
Owner

@char0n char0n left a comment

Choose a reason for hiding this comment

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

@mathieutu thanks for bringing this up. Though I agree with the premise of this function supporting objects (which it does), we have do a little bit more then just adding overloaded type def.

IMHO we have to add additional sentence: Filterable objects include plain objects or any object that has a filter method such as Array.. Along with that we have to change description in source file, add new examples and incorporate new tests.

Would you like to take this over, or should I create a separate ticket for this ?

@mathieutu
Copy link
Contributor Author

mathieutu commented Oct 4, 2020

Hi @char0n
I agree with all of that.

I think we could iterate on it : merging this one to fix the type issue to begin, and making a ticket to handle the other changes afterwards.

@char0n
Copy link
Owner

char0n commented Oct 4, 2020

@mathieutu well along with typing you also changed the description of the function.

If you amend the PR and return the description to original state I'll be willing to just change the typing to add support for Object. If we want to change the description also we'll have to proceed as I suggested.

@char0n
Copy link
Owner

char0n commented Oct 4, 2020

On another note, what is the difference between TS Dictionary and Record? Why prefer the Dictionary? Thanks for enlightening me ;]

types/index.d.ts Outdated Show resolved Hide resolved
@mathieutu
Copy link
Contributor Author

It's done.

Dictionnary is an internal type of ramda adjunct, it's why I've used it:

    interface Dictionary<T> { [key: string]: T; }

It's exactly the same than Record<string, T>.

@mathieutu
Copy link
Contributor Author

mathieutu commented Oct 4, 2020

I've updated the type to follow #1603 changes, and improved it in cas of complex objects. It return the same keys but without falsy values.
Screenshot 2020-10-07 at 18 19 34

@mathieutu mathieutu requested a review from char0n October 7, 2020 16:06
@char0n
Copy link
Owner

char0n commented Oct 8, 2020

@BenoitHiller
Copy link
Contributor

Sticking my head in here because my PR got mentioned and I see this is still open.

With the latest changes here you should just delete this line: https://github.com/char0n/ramda-adjunct/pull/1611/files#diff-88bec6beae84369c0376b56b3bb88fe1R1190

Arrays are objects and so the index signature you have there works just as well for them.

The two failing tests are https://github.com/char0n/ramda-adjunct/blob/master/types/test/compact.ts#L15 and https://github.com/char0n/ramda-adjunct/blob/master/types/test/compact.ts#L18. Your change makes them no longer correct so you should just delete them (npm run test:types to run the type tests. this isn't documented anywhere).

I personally would also add in some tests for filtering objects as it gets pretty complicated sometimes.

@char0n
Copy link
Owner

char0n commented Dec 12, 2020

@mathieutu are you willing to continue on this PR as @BenoitHiller suggested?

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.

3 participants