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

feat: Draft fledgeling object, ready for internal use #581

Merged
merged 26 commits into from
Nov 10, 2022
Merged

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Nov 10, 2022

Closes #402.

TODOS

  • Also parse h2 for version (see pkgdown)
  • tests

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

@maelle: Can you please pick up from here?

R/fledgeling.R Outdated Show resolved Hide resolved
R/fledgeling.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@maelle
Copy link
Member

maelle commented Nov 10, 2022

@krlmlr I added a bit more parsing, and tests, you can now review this.

@maelle
Copy link
Member

maelle commented Nov 10, 2022

Working on writing should happen after both this and #584 are merged, but I'm working on paper in the meantime.

I feel parsing the news a bit more should happen now too.

R/fledgeling.R Outdated Show resolved Hide resolved
@maelle
Copy link
Member

maelle commented Nov 10, 2022

why is the object immutable, more specifically, why not copy more of desc logic?

Copy link
Contributor Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks. The h2 detection is broken, but we also don't need it here. Can you please remove it? Parsing the individual h1 sections would be done on demand, later.

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

Maybe I misunderstood h2.

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

Can you share an example NEWS file where the h2 detection is triggered?

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

The original column should perhaps contain the entire section, including empty lines. Perhaps as a single character string, with embedded "\n" ?

@maelle
Copy link
Member

maelle commented Nov 10, 2022

why would the header and content be stored together?

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

Should the class name be "fledge_ling" ? So that the package name still is the prefix?

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

I have code that also extracts the preamble: https://github.com/cynkra/fledge/tree/f-402-fledgeling-3 .

@maelle
Copy link
Member

maelle commented Nov 10, 2022

Should the class name be "fledge_ling" ? So that the package name still is the prefix?

but what about method names?

@maelle
Copy link
Member

maelle commented Nov 10, 2022

I have code that also extracts the preamble: f-402-fledgeling-3 .

why not add it to this branch?

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

To avoid conflicts with your work. Pushing now.

@maelle
Copy link
Member

maelle commented Nov 10, 2022

ok, sorry, I'm experimenting with Pandoc in no branch :-)

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

Also keeping raw text now for preamble and all sections. We can do a perfect roundtrip with this object now, and should start using it everywhere in the codebase.

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 10, 2022

Fixed corner case with empty preamble.

Ready for review, talk soon!

Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

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

should this be merged?

@krlmlr krlmlr mentioned this pull request Nov 10, 2022
@krlmlr krlmlr changed the title WIP: Draft fledgeling object feat: Draft fledgeling object, ready for internal use Nov 10, 2022
@krlmlr krlmlr merged commit f218590 into main Nov 10, 2022
@krlmlr krlmlr deleted the f-402-fledgeling branch November 10, 2022 14:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Simple fledgling object
2 participants