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

Tim-Siu · 2024-04-07T03:29:14Z

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: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

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):

To be included in the release note for any feature that is made obsolete/breaking

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 · 2024-04-07T03:44:28Z

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 · 2024-04-07T03:46:29Z

@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

Tim-Siu · 2024-04-07T03:47:00Z

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 · 2024-04-07T03:52:44Z

@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:

I will add related documentation when the desired behaviors are agreed upon.
To my best knowledge, testing for logging is currently not available?
I failed to mention a previous related PR. Thank you for the reminding.
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 · 2024-04-07T04:00:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yucheng11122017 · 2024-04-07T05:30:23Z

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

Hi @Tim-Siu thanks for the work!

One clarification: is the intention also to add a logging statement if the included file is empty?

Eg.
In trial.md

<includes src="trialInclude.md" />

while trialInclude.md is present but the file is empty.

This behaviour doesn't trigger a warning right now.

packages/core/src/html/includePanelProcessor.ts

yucheng11122017 · 2024-04-09T03:26:49Z

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>

itsyme · 2024-04-09T09:01:44Z

packages/core/src/html/includePanelProcessor.ts

@@ -66,7 +66,7 @@ function _getBoilerplateFilePath(element: MbNode, filePath: string, config: Reco
 function _getSrcFlagsAndFilePaths(element: MbNode, config: Record<string, any>) {
  const isUrl = urlUtil.isUrl(element.attribs.src);

-  // We do this even if the src is not a url to get the hash, if any
+  // We do this even if the src is not an url to get the hash, if any


'a' is actually the correct word as url is pronounced as 'you-ar-el' which leads to the first "sound" letter being 'y'. This article explains in more detail https://owl.purdue.edu/owl/general_writing/grammar/articles_a_versus_an.html

Sorry for this. It was highlighted by static analysis and I thought it was a valid typo HAHA

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Tim-Siu · 2024-04-13T16:32:01Z

HI @yucheng11122017

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

yucheng11122017

Small nit

yucheng11122017 · 2024-04-13T17:49:08Z

docs/userGuide/syntax/siteNavigationMenus.md

-<div id="examples"></div>
+<div id="examples" class="d-none">
+
+You can see an example of a Site Navigation Menu ==on the top== of <a target="_blank" href="{{ baseUrl }}/userGuide/formattingContents.html">this page</a>.


Er isn't it on the left?

yucheng11122017

LGTM!

github-actions · 2024-04-18T07:31:27Z

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

…2506)

Tim-Siu added 2 commits April 7, 2024 11:07

Add warning for empty segment includes

b543339

Add warning for empty includes

d0e7b69

Tim-Siu added 3 commits April 8, 2024 11:40

Merge branch 'master' into include-log

353300c

Update the behaviours

0a704b2

Remove legacy code

12e6c10

yucheng11122017 reviewed Apr 9, 2024

View reviewed changes

packages/core/src/html/includePanelProcessor.ts Outdated Show resolved Hide resolved

itsyme reviewed Apr 9, 2024

View reviewed changes

Tim-Siu and others added 5 commits April 13, 2024 09:50

Merge branch 'master' into include-log

6956757

Apply suggestions from code review

cc6b8cb

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Add examples in siteNavigationMenus.md

ed5e8b1

Warn empty include file

e818a57

Fix typo

e2424b1

Fix redundant example

fef8ced

yucheng11122017 reviewed Apr 13, 2024

View reviewed changes

Tim-Siu and others added 2 commits April 15, 2024 12:39

Merge branch 'master' into include-log

799e2cc

Fix wrong information

9cd7de9

yucheng11122017 approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'master' into include-log

2060369

KevinEyo1 merged commit 104b46c into MarkBind:master Apr 18, 2024
7 checks passed

KevinEyo1 added the r.Patch Version resolver: increment by 0.0.1 label Apr 18, 2024

itsyme pushed a commit that referenced this pull request Apr 19, 2024

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

ac3e4f5

…2506)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Tim-Siu commented Apr 7, 2024 •

edited by KevinEyo1

yucheng11122017 commented Apr 7, 2024

yucheng11122017 commented Apr 7, 2024 •

edited

Tim-Siu commented Apr 7, 2024

Tim-Siu commented Apr 7, 2024

codecov bot commented Apr 7, 2024 •

edited

yucheng11122017 commented Apr 7, 2024 •

edited

yucheng11122017 left a comment

yucheng11122017 commented Apr 9, 2024

itsyme Apr 9, 2024

Tim-Siu Apr 13, 2024

Tim-Siu commented Apr 13, 2024

yucheng11122017 left a comment

yucheng11122017 Apr 13, 2024

yucheng11122017 left a comment

github-actions bot commented Apr 18, 2024

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

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

Conversation

Tim-Siu commented Apr 7, 2024 • edited by KevinEyo1

Add warning for including empty segments or files in optional mode

yucheng11122017 commented Apr 7, 2024

yucheng11122017 commented Apr 7, 2024 • edited

Tim-Siu commented Apr 7, 2024

Tim-Siu commented Apr 7, 2024

codecov bot commented Apr 7, 2024 • edited

Codecov Report

yucheng11122017 commented Apr 7, 2024 • edited

yucheng11122017 left a comment

Choose a reason for hiding this comment

yucheng11122017 commented Apr 9, 2024

itsyme Apr 9, 2024

Choose a reason for hiding this comment

Tim-Siu Apr 13, 2024

Choose a reason for hiding this comment

Tim-Siu commented Apr 13, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

yucheng11122017 Apr 13, 2024

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2024

Tim-Siu commented Apr 7, 2024 •

edited by KevinEyo1

yucheng11122017 commented Apr 7, 2024 •

edited

codecov bot commented Apr 7, 2024 •

edited

yucheng11122017 commented Apr 7, 2024 •

edited