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 Base.one for StringView #24

Merged
merged 12 commits into from
Apr 25, 2024

Conversation

mnemnion
Copy link
Contributor

@mnemnion mnemnion commented Mar 3, 2024

I have a lot of S where S<:AbstractString code which uses one(S) for the empty string to compare against. I didn't realize until I went to add this that typemin also works, but hey, that makes the implementation that much easier, and StringViews gives me a zero-copy drop-in replacement for a string buffer with this addition.

I have a lot of `S where S<:AbstractString` code which uses `one(S)` for the empty string to compare against. I didn't realize until I went to add this that `typemin` also works, but hey, that makes the implementation that much easier, and StringViews gives me a zero-copy drop-in replacement for a string buffer with this addition.
@stevengj
Copy link
Member

stevengj commented Mar 4, 2024

Looks good to me, can you add a test?

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.08%. Comparing base (7c7f0b6) to head (83796dc).
Report is 24 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   75.05%   77.08%   +2.02%     
==========================================
  Files           6        6              
  Lines         417      467      +50     
==========================================
+ Hits          313      360      +47     
- Misses        104      107       +3     
Flag Coverage Δ
unittests 77.08% <100.00%> (+2.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

And a test for identity equality
@mnemnion
Copy link
Contributor Author

mnemnion commented Mar 5, 2024

Done. I admittedly have been making these edits through the GitHub interface, after trying them in the REPL. It looks like tests run under CI? I'm willing to clone and run locally if necessary.

src/StringViews.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
mnemnion and others added 4 commits March 7, 2024 10:17
Co-authored-by: Steven G. Johnson <[email protected]>
Co-authored-by: Steven G. Johnson <[email protected]>
Also defined for Strings
@mnemnion
Copy link
Contributor Author

mnemnion commented Mar 7, 2024

Good catch, a careful look at the documentation also turned up oneunit, which doesn't fall back on one, so I defined it also.

src/StringViews.jl Outdated Show resolved Hide resolved
As well as a custom typemin for StringView(Base.CodeUnits{etc}}.

This makes the Base implementation of typemin function for both
s and su in the test suite, and I would consider returning the
data field for an attempt to convert a StringView to its underlying
data to be legitimate.
@mnemnion
Copy link
Contributor Author

mnemnion commented Mar 18, 2024

This commit got rid of the custom oneunit, and one/oneunit/typemin all work on StringView{Base.CodeUnits{UInt8,String}} due to an added special case.

It adds a convert to the StringView's underlying data type from the StringView, which I consider legitimate. I didn't add a test for this because the code pathway is exercised by the typemin tests, but adding one would be pretty easy, if the maintainers consider the convert legitimate in the first place.

@mnemnion
Copy link
Contributor Author

Just chiming in to say that I intend to squash all of this into one commit whenever you think it's in a good state to merge. I ended up checking in some spurious data among other things.

Speaking of which, would you accept a separate PR which adds Manifest.toml to a .gitignore? Can't hurt.

I'm not in a rush here, to be clear. I ended up switching two calls to use typemin instead of one, which works fine for my application, and means I use a better lower bound on the dependency.

@stevengj
Copy link
Member

I don't like including a Manifest.toml file in the repo, but .gitignore is fine.

src/StringViews.jl Outdated Show resolved Hide resolved
@mnemnion
Copy link
Contributor Author

I don't like including a Manifest.toml file in the repo, but .gitignore is fine.

Right, I removed it once I realized it came from me. The .gitignore is simply to prevent that happening in future.

Manifest.toml Outdated Show resolved Hide resolved
@mnemnion
Copy link
Contributor Author

Would you say this is ready to go? I'd like to squash it, so the spurious check-ins don't end up in the tree. I've held off because that should really be done once.

@stevengj stevengj merged commit 292a9cc into JuliaStrings:main Apr 25, 2024
9 checks passed
@stevengj
Copy link
Member

Thanks, I squashed and merged.

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