Skip to content

refactor: Move and group related code (Part 4/X)#103

Merged
Byron merged 2 commits intoByron:mainfrom
ConnorGray:connorgray/refactor-4
Feb 21, 2025
Merged

refactor: Move and group related code (Part 4/X)#103
Byron merged 2 commits intoByron:mainfrom
ConnorGray:connorgray/refactor-4

Conversation

@ConnorGray
Copy link
Contributor

@ConnorGray ConnorGray commented Feb 20, 2025

Stacked PRs:


My impression reading through the code in this project is that top-level statements have gotten a bit intertangled over time; this tries to reorder things to have a clearer flow and structure, grouping related things together.

For example, this moves the recently added Error enum up above all of the API functions, instead of leaving it somewhat arbitrarily nestled between cmark_resume_with_options() and cmark_resume_one_event().

No code changes were made to the body of functions or types. This also does not change any of the public APIs, only where code is located.

  • Group padded newline logic at top of text_modifications.rs

  • Rename padding_of() to list_item_padding_of()

  • Add 'Public API Functions' header in lib.rs

  • Consolidate public enum and struct types at the top of lib.rs, instead of leaving them scattered amongst the cmark*() public functions.

  • Consolidate the two impl State<'_> statements

  • Move cmark_resume(), cmark() and cmark_with_options() from the bottom of lib.rs up to the top, before the ~600 implementation of cmark_resume_one_event() that makes up the bulk of lib.rs

My impression reading through the code in this project is that
top-level statements have gotten a bit intertangled over time;
this tries to reorder things to have a clearer flow and
structure, grouping related things together.

For example, this moves the recently added Error enum up above
all of the API functions, instead of leaving it somewhat
arbitrarily nestled between `cmark_resume_with_options()` and
`cmark_resume_one_event()`.

No code changes were made to the body of functions or types.
This also does not change any of the public APIs, only where
code is located.

* Group padded newline logic at top of text_modifications.rs

* Rename `padding_of()` to `list_item_padding_of()`

* Add 'Public API Functions' header in lib.rs

* Consolidate public enum and struct types at the
  top of lib.rs, instead of leaving them scattered
  amongst the `cmark*()` public functions.

* Consolidate the two `impl State<'_>` statements

* Move `cmark_resume()`, `cmark()` and `cmark_with_options()`
  from the bottom of lib.rs up to the top, before the ~600
  implementation of `cmark_resume_one_event()` that makes up
  the bulk of lib.rs
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot, that makes sense and certainly helps with navigating the code while perusing it.

I left a comment about labels/flags, and that additional modularisation should be used instead. This could be left done in a separate commit, separate PR, or left out entirely

Thanks again.

src/lib.rs Outdated
}
}

//======================================
Copy link
Owner

Choose a reason for hiding this comment

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

Could all labels (like these) be removed?
If further structuring is required, functions can be moved into their own modules that are named accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course 🙂 That's a bit of my own personal style leaking through a bit too much; no problem with removing these :)

I just pushed a commit that drops these. Let me know if there's anything else! :)

@ConnorGray ConnorGray requested a review from Byron February 20, 2025 13:29
@Byron
Copy link
Owner

Byron commented Feb 21, 2025

Thanks a lot, this works!

@Byron Byron merged commit 3005f1b into Byron:main Feb 21, 2025
2 checks passed
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