Hi Amahnui,

On 4/27/22 19:27, Abongwa Amahnui Bonalais wrote:> Signed-off-by: Abongwa Bonalais Amahnui <abongwabonal...@gmail.com>> ---


Adding a comment in the commit log to explain why this patch is needed and what it does would be great.

  scripts/docs_fix_all_html_css.py | 111 +++++++++++++++++++++++++++++++
  scripts/run-docs-build           |   2 +
  2 files changed, 113 insertions(+)
  create mode 100644 scripts/docs_fix_all_html_css.py

diff --git a/scripts/docs_fix_all_html_css.py b/scripts/docs_fix_all_html_css.py
new file mode 100644
index 0000000..db99054
--- /dev/null
+++ b/scripts/docs_fix_all_html_css.py
@@ -0,0 +1,111 @@
+#!/usr/bin/env python3
+#
+# SPDX-License-Identifier: GPL-2.0-only
+#
+#Signed-off-by: Abongwa Bonalais Amahnui <abongwabonal...@gmail.com>
+#
+#
+# function to append to the content of a html file below the body tag
+#

Please add a comment as to what this script is supposed to to. You're discussing the implementation here more than the finality. We need to know when opening the file and reading the comment what this should/would be used for.

+#
+
+import os
+
+
+
+html_content_dunfell = '''
+<div id="outdated-warning">This document is outdated, you should select the <a 
href="https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.yoctoproject.org_&amp;d=DwMDAg&amp;c=_sEr5x9kUWhuk4_nFwjJtA&amp;r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&amp;m=aRH3jUhIOsO6ovQMKiTHiW-F3Xwnx9HWg6yTF99QSvNBJXbkOkdzSNhwGt6I4zHl&amp;s=QnWAweVmLt12aTlVPVFZsjAeYelStsO_8RsqIskX0Sk&amp;e=";>latest
 release version</a> in this series.</div>
+<div xml:lang="en" class="body" lang="en">
+'''

I would nitpick here and say we should probably point to https://docs.yoctoproject.org/dunfell in the href. But that can be fixed/discussed later.

+html_content = '''
+<div id="outdated-warning">This version of the project is now considered obsolete, please select and use a <a 
href="https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.yoctoproject.org&amp;d=DwMDAg&amp;c=_sEr5x9kUWhuk4_nFwjJtA&amp;r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&amp;m=aRH3jUhIOsO6ovQMKiTHiW-F3Xwnx9HWg6yTF99QSvNBJXbkOkdzSNhwGt6I4zHl&amp;s=XxAPqWFTh9XMmtK4eywrsIFkuOGfwEs8acOxvNGck2g&amp;e=";>more
 recent version</a>.</div>
+<div xml:lang="en" class="body" lang="en">
+'''
+
+# <div xml:lang="en" class="body" lang="en"> and </div> are added to the html 
files to wrap all the content below the body tag in a div tag whose class is known so it can be controlled in the css file

You are explaining what you are doing, which we can usually understand by reading the code, instead of explaining why you're doing it which is something we cannot guess. Why was it not possible to just insert the banner in the body tag as a div before any other element/tag?

+last_div = '''
+</div>
+
+'''
+
+css_replacement_content = '''
+
+  font-family: Verdana, Sans, sans-serif;
+
+  /*min-width: 640px;*/

You can remove this commented part as it does not provide any context or help.

+  width: 100%;
+  margin:  0;
+  padding: 0;
+  color: #333;
+  overflow-x: hidden;
+  }
+
+ /*added books too*/

Not sure what I am supposed to understand with this comment?

+.body{
+margin:  0 auto;
+min-width: 640px;
+padding: 0 5em 5em 5em;
+}
+/* added the id below to make the banner show and be fixed*/
+#outdated-warning{
+text-align: center;
+background-color: rgb(255, 186, 186);
+color: rgb(106, 14, 14);
+padding: 0.5em 0;
+width: 100%;
+position: fixed;
+top: 0;
+
+
+'''
+    # pattern = '^3.1*'
+    # exclude = re.search(pattern, dir)

Useless comments, please remove.

+def loop_through_html_directories(dir):
+    exclude = []
+    for root, dirs, filenames in os.walk(dir):
+         # exclude banner for 3.1.x upward as it is an LTS release and is 
still supported
+        exclude = [ name for name in os.listdir(dir) if name.startswith('3.1') 
]
+        for d in dirs:
+            if d in exclude:
+                dirs.remove(d)
+        for filename in filenames:
+            if filename.endswith('.html'):
+                with open(os.path.join(root, filename), 'r', 
encoding="ISO-8859-1") as f:
+                    current_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+                    f.write(current_content.replace('<body>', '<body>' + 
html_content))
+                    f.write(current_content.replace('</body>', last_div + 
'</body>'))
+            if filename.endswith('.css'):
+                with open(os.path.join(root, filename), 'r', 
encoding="ISO-8859-1") as f:
+                    css_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+                    
f.write(css_content.replace(css_content[css_content.find('body 
{'):css_content.find('}'[0])], 'body {' + css_replacement_content ))
+    print(exclude)
+loop_through_html_directories('.')
+

The function name is not really helpful, you're explaining the implementation more than the outcome of it. I should call this function if I want to add a banner for olds docs. I could suggest add_banner_old_docs as a function name instead, though other people could probably come up with something more meaningful.

+
+
+
+def dunfell_tags(dir):
+    dunfell_banners = []
+    for root, dirs, filenames in os.walk(dir):
+        dunfell_banners = [ name for name in os.listdir(dir) if not 
name.startswith('3.1') ]
+        for d in dirs:
+            if d in dunfell_banners:
+                dirs.remove(d)
+
+        for filename in filenames:
+            if filename.endswith('.html'):
+                with open(os.path.join(root, filename), 'r', 
encoding="ISO-8859-1") as f:
+                    current_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+                    f.write(current_content.replace('<body>', '<body>' + 
html_content_dunfell))
+                    f.write(current_content.replace('</body>', last_div + 
'</body>'))
+            if filename.endswith('.css'):
+                with open(os.path.join(root, filename), 'r', 
encoding="ISO-8859-1") as f:
+                    css_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+                    
f.write(css_content.replace(css_content[css_content.find('body 
{'):css_content.find('}'[0])], 'body {' + css_replacement_content ))
+    print(dunfell_banners)

This is all extremely complex just for using a different variable depending on the name of the directory.

+def add_banner_old_docs(dir):
+    exclude = []
+    for root, dirs, filenames in os.walk(dir):
+        if root.startswith('./3.1'):
+            html_replacement = html_content_dunfell
+        else:
+            html_replacement = html_content
+
+        for filename in filenames:
+            if filename.endswith('.html'):
+ with open(os.path.join(root, filename), 'r', encoding="ISO-8859-1") as f:
+                    current_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+ f.write(current_content.replace('<body>', '<body>' + html_replacement)) + f.write(current_content.replace('</body>', last_div + '</body>'))
+            if filename.endswith('.css'):
+ with open(os.path.join(root, filename), 'r', encoding="ISO-8859-1") as f:
+                    css_content = f.read()
+                with open(os.path.join(root, filename), 'w') as f:
+ f.write(css_content.replace(css_content[css_content.find('body {'):css_content.find('}'[0])], 'body {' + css_replacement_content ))
+    print(exclude)
+add_banner_old_docs('.')

Cheers,
Quentin
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#56974): https://lists.yoctoproject.org/g/yocto/message/56974
Mute This Topic: https://lists.yoctoproject.org/mt/90736059/21656
Group Owner: yocto+ow...@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/yocto/unsub 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to