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

[DRAFT] Update docstrings and allow AbstractString #7

Merged
merged 4 commits into from
Jul 27, 2024

Conversation

svilupp
Copy link
Collaborator

@svilupp svilupp commented Jul 27, 2024

A lot of operations might produce non-standard strings (eg InlineString, SubString{String}), this PR extends the support to such strings as well.
Added a mini test for each just to show that it works.

It also adds docstrings for the two main functions, so people can use the help mode.

This PR should be merged only AFTER we merge the restructure (I'll move things around once that happens)

Some minor changes, eg,

  • moved _load_prefix_file to be an external function, otherwise it gets defined again every time you call Prefixes inside of the split_sentence.
  • changed unprovided kwargs to be nothing as opposed to missing (missing is intended more when there is a data point but we don't know what it is versus nothing is meant when nothing is provided like no argument)
  • x!=nothing changed to more idiomatic !isnothing(x) (similar for isempty)

EDIT: sorry, there are a few more "changes" -- I have an auto-formatter enabled. I hope it's okay since there is so little code anyway. We'll re-format everything in the end anyway.

@svilupp svilupp assigned svilupp and astariul and unassigned svilupp Jul 27, 2024
@svilupp
Copy link
Collaborator Author

svilupp commented Jul 27, 2024

@astariul this PR is ready for review. Let me know what you think.

There will be 5more steps:

  • I'll create formatting PR to align to sciml format
  • YOU should enable github pages (now that docs are getting created)
  • YOU should go to codecov.io and enter the codecov secret token into the environment secrets so we can report the test coverage etc.
  • YOU need to enable JuliaRegistrator bot on this repo (click on "Install app" here: https://github.com/JuliaRegistries/Registrator.jl/#registrator)
  • (last) YOU can then register by commenting on the latest merged commit with @JuliaRegistrator register

Copy link
Owner

@astariul astariul left a comment

Choose a reason for hiding this comment

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

@astariul
Copy link
Owner

@svilupp This is awesome work, thanks for efforts !

I did all the steps that you conveniently listed, thanks for that.

@astariul astariul merged commit 0468142 into astariul:main Jul 27, 2024
5 checks passed
@svilupp
Copy link
Collaborator Author

svilupp commented Jul 27, 2024

@astariul sorry, you need to comment on the commit that gets merged to main! not the pull request.

Ie, go to the main page of your repo, click on the clock like symbol of in the top right corner (just above the folders) -> select the last commit you made (this PR) -> scroll to the bottom and there you can paste the message to JuliaRegistrator.

You should get a reply in 10 seconds that it worked.

It will be registered in 3 days from now

@svilupp
Copy link
Collaborator Author

svilupp commented Jul 27, 2024

@astariul also, if you can wait 5 minutes, I’ll publish the reformated code!

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.

2 participants