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

Implement deleteat! and append! #23

Merged
merged 2 commits into from
Jun 3, 2019
Merged

Conversation

nalimilan
Copy link
Member

Fixes #22.

@nalimilan nalimilan changed the title Implement deleteat! Implement deleteat! and append! May 17, 2019
Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

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

Looks good. append! is probably even more safe than what we have in _vcat below 😄.

@nalimilan
Copy link
Member Author

This doesn't support append! from a generic iterable, but I think fallback methods should be defined in Base since it already contains methods which use resize! and setindex! or push!, which are perfectly generic. It sounds silly to redefine these in all packages.

@bkamins
Copy link
Member

bkamins commented May 17, 2019

The problem is that in Julia 1.0 this is not present so if we aim at long-term support maybe we should conditionally define them (and now they would always be defined in the package)?

itemindices = eachindex(items)
l = length(pv)
n = length(itemindices)
resize!(pv.refs, l+n)
Copy link
Member Author

Choose a reason for hiding this comment

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

This would ensure entries are set to #undef in case conversion fails for some values:

Suggested change
resize!(pv.refs, l+n)
resize!(pv, l+n)

That's not what happens for Arrays of isbits types, but maybe that's worth it here given that PooledArray is mostly useful for non-isbits types? Otherwise, leaving refs undefined could trigger crashes.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that you are afraid that later copyto! will not set the proper values there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if conversion fails it will stop in the middle AFAIK.

@nalimilan
Copy link
Member Author

The problem is that in Julia 1.0 this is not present so if we aim at long-term support maybe we should conditionally define them (and now they would always be defined in the package)?

Yeah, but honestly I don't really care. :-)

At least let's get this included in Base first so that we know what VERSION check to use: JuliaLang/julia#32065.

@bkamins
Copy link
Member

bkamins commented May 17, 2019

I think that we should care :), I answer SO a lot, and people report there often that they are stuck with 1.0 when I give them some solutions that use 1.1 features.

@bkamins
Copy link
Member

bkamins commented May 18, 2019

Also insert!, splice! and prepend! should be added to be consistent with Vector API.

@nalimilan
Copy link
Member Author

Bump.

@bkamins
Copy link
Member

bkamins commented May 26, 2019

I think it is good to merge. Adding other Vector operations that are still undefined can be done in a separate PR (changes in JuliaData/WeakRefStrings.jl#63 gives a list what should be implemented)

@bkamins
Copy link
Member

bkamins commented Jun 1, 2019

bump (as this package is getting used more by the community and the problem is repeatedly reported)

@nalimilan
Copy link
Member Author

@shashi @JeffBezanson OK to merge?

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Looks good. I forgot I have access rights here.

@quinnj quinnj merged commit c30a671 into JuliaData:master Jun 3, 2019
@bkamins
Copy link
Member

bkamins commented Jun 3, 2019

@quinnj can you also please tag a release then? Thank you.

@nalimilan nalimilan deleted the nl/deleteat! branch June 3, 2019 20:56
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.

deleteat! missing
3 participants