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

Read mdocfile data from a string #28

Merged
merged 7 commits into from
Aug 15, 2024
Merged

Conversation

daniel-ji
Copy link
Contributor

Resolves #27. Can now call read_string(mdoc_file_data) in addition to read(mdoc_file)

@alisterburt
Copy link
Member

Awesome PR, thanks so much for taking the time!

I love the class method but not so much the top level API of read_string. Could you remove it? This would give you Mdoc.from_string().as_dataframe() in your own scripts/applications which doesn't feel too onerous, what do you think? 🙂

@alisterburt
Copy link
Member

Whoops, sorry I thought the API already had Mdoc.as_dataframe(), could you move your _read helper into that method on the Mdoc class?

@daniel-ji
Copy link
Contributor Author

daniel-ji commented Aug 14, 2024

Sounds good, I'll go ahead and remove the added documentation as well. And maybe mention this new method in the documentation (or not)?

@alisterburt
Copy link
Member

Documenting this is a good idea!👍

@daniel-ji
Copy link
Contributor Author

Done, let me know if I should change / improve anything!

@daniel-ji
Copy link
Contributor Author

Particularly the usage, is this okay?

from mdocfile.data_models import Mdoc

mdoc_data = ...

mdoc = Mdoc.from_string(mdoc_data).as_dataframe()

@alisterburt
Copy link
Member

Looks great! In an extra section "parsing from text"

@daniel-ji
Copy link
Contributor Author

Sounds good! I've updated the README and index.md files, let me know if I should only be changing one or if I should be adding a new page or something like that. Thanks!

@daniel-ji
Copy link
Contributor Author

daniel-ji commented Aug 15, 2024

Hi, @alisterburt, could this get merged in? And was wondering if this could be included in a release anytime soon? Thanks!

Copy link
Member

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the PR @daniel-ji 🙂 really great work!

@alisterburt alisterburt merged commit 517bc8d into teamtomo:main Aug 15, 2024
5 checks passed
@alisterburt
Copy link
Member

@daniel-ji I can push a release but it won't be for a few days at least - I'm currently on vacation 🙂

Maybe @brisvag or @jojoelfe can push a release here? Unsure who has permissions

(Would do it but I'm on mobile)

@jojoelfe
Copy link
Contributor

I would, but doesn't look like I can...

@daniel-ji
Copy link
Contributor Author

daniel-ji commented Aug 15, 2024

No worries, it can wait awhile, sorry for all the bothering. I hope you enjoy your vacation!

@brisvag
Copy link
Contributor

brisvag commented Aug 16, 2024

I pushed a tag, but we have no CI publish workflow it seems. I cannot do it manually cause I'm not maintainer on pypi, nor member of the teamtomo org there... I think we need to set up trusted publishing and all that :P

@alisterburt
Copy link
Member

But... there is a CI publish workflow? ci.yml - i don't see the tag pushed

@brisvag
Copy link
Contributor

brisvag commented Aug 16, 2024

Bizarre... There was some weirdness with git, I could see the tag on the remote and it said "all up to date" so I assume the tag was pushed. For some reason I had to explicitly use git push upstream v0.1.3 to make it work, --follow-tags was not enough O.o Anyways, should be good now!

@alisterburt
Copy link
Member

Thanks for getting back to it so fast @brisvag ! Congratulations @daniel-ji on your first PR to the project and to teamtomo, your changes have been released in v0.1.3 https://github.com/teamtomo/mdocfile/actions/runs/10419687859

@daniel-ji
Copy link
Contributor Author

daniel-ji commented Aug 16, 2024

Awesome, thank you guys all so much for the quick and fantastic work!

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.

Read mdoc filedata from string (not filename)
4 participants