search

MarkBind / markbind

MarkBind is a tool for generating content-heavy websites from source files in Markdown format
https://markbind.org/
MIT License
134 stars 123 forks source link

Add warning for including empty segments or files in optional mode #2506

Closed Tim-Siu closed 2 months ago

Tim-Siu commented 3 months ago

What is the purpose of this pull request?

Overview of changes: Related to #2180 Resolve #2181 Add warning message in the case that empty files or segments are included in the <include .... optional/> case.

Anything you'd like to highlight/discuss:

Testing instructions:

Minimal reproduction:

<div id="empty-segment">
<include src="some_path#empty" optional/>
</div>

<div id="empty">
<include src="some_path" optional/>
</div>

The log warning expected:

warn: Optional segment '#empty' not found in file: ...
Empty reference in ...
warn: Optional file not found: ...
Empty reference in ...

Proposed commit message: (wrap lines at 72 characters)

Add warning for including empty segments or files in optional mode

Checklist: :ballot_box_with_check:

Reviewer checklist:

Indicate the SEMVER impact of the PR:

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
yucheng11122017 commented 3 months ago

Hi @Tim-Siu, is this the desired behaviour? My impression from reading the issue and the comments is that there is a need for a warning message if the include content is empty, regardless of whether the fragment is optional or not.

In other words, 

<includes src="path#empty" />
<div id="empty" />

will log a warning regardless of whether the include is optional or not.

yucheng11122017 commented 3 months ago

@Tim-Siu Also once again, please look at the checklist to see if you missed out on anything and check it off if you have done it.

Checklist: :ballot_box_with_check:

  • [ ] Updated the documentation for feature additions and enhancements
  • [ ] Added tests for bug fixes or features
  • [ ] Linked all related issues
  • [ ] No unrelated changes
Tim-Siu commented 3 months ago

Hi @Tim-Siu, is this the desired behaviour? My impression from reading the issue and the comments is that there is a need for a warning message if the include content is empty, regardless of whether the fragment is optional or not.

In other words,

<includes src="path#empty" />
<div id="empty" />

will log a warning regardless of whether the include is optional or not.

Hi @yucheng11122017 ,

Currently empty includes without optional attributes will lead to error (and will be printed to both console and rendered web pages).

Tim-Siu commented 3 months ago

@Tim-Siu Also once again, please look at the checklist to see if you missed out on anything and check it off if you have done it.

Checklist: ☑️

  • [ ] Updated the documentation for feature additions and enhancements
  • [ ] Added tests for bug fixes or features
  • [ ] Linked all related issues
  • [ ] No unrelated changes

Hi @yucheng,

Regarding the checklist:

  1. I will add related documentation when the desired behaviors are agreed upon.
  2. To my best knowledge, testing for logging is currently not available?
  3. I failed to mention a previous related PR. Thank you for the reminding.
  4. I Fixed a small typo in the same code file. I will remove the fix if it is against conventional practices.

Thank you once again for your comments.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 51.03%. Comparing base (8253f82) to head (cc6b8cb).

:exclamation: Current head cc6b8cb differs from pull request most recent head 2060369. Consider uploading reports for the commit 2060369 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2506 +/- ## ========================================== + Coverage 50.99% 51.03% +0.03% ========================================== Files 124 124 Lines 5383 5387 +4 Branches 1160 1163 +3 ========================================== + Hits 2745 2749 +4 Misses 2348 2348 Partials 290 290 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yucheng11122017 commented 3 months ago

Hi @Tim-Siu, is this the desired behaviour? My impression from reading the issue and the comments is that there is a need for a warning message if the include content is empty, regardless of whether the fragment is optional or not. In other words,

<includes src="path#empty" />
<div id="empty" />

will log a warning regardless of whether the include is optional or not.

Hi @yucheng11122017 ,

Currently empty includes without optional attributes will lead to error (and will be printed to both console and rendered web pages).

Hi @Tim-Siu, I believe that the purpose of #2181 is to log when a user includes on a fragment that is empty.

You can take a look at the test case added by the original PR which lead to this issue. The idea is that if there a empty fragment like <div id="empty />, there should be a log that tells the user they are including an empty fragment.

Eg. In trial.md

<includes src="trialInclude.md#empty" />

In trialInclude.md

<div id="empty" />

Because the div is empty, we add a log statement for the user.

Right now, you are logging only if the fragment does not exist and optional is enabled. Eg.

In trial.md

<include src="trialInclude.md#missing" optional />

In trialInclude.md there is no tag with missing id.

We don't need this behaviour when optional is enabled because the whole idea is that if optional is enabled, the fragment can be skipped. Otherwise, the user shouldn't have enabled optional in the first place.

Please let me know if you don't understand - Will be happy to explain again

yucheng11122017 commented 3 months ago

Could you also fix the fact that the #example in site navigation menu being empty so that the build doc doesn't fail?

It can be similiar to the one for page nav

<div id="examples" class="d-none">

You can see an example of a Page Navigation Bar ==on the right side== of <a target="_blank" href="{{ baseUrl }}/userGuide/formattingContents.html">this page</a>.
</div>
Tim-Siu commented 2 months ago

HI @yucheng11122017

Thank you for your suggestions. I have made changes accordingly.

github-actions[bot] commented 2 months ago

@KevinEyo1 Each PR must have a SEMVER impact label, please remember to label the PR properly.