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

added data to app breadcrumb #157

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

added data to app breadcrumb #157

wants to merge 4 commits into from

Conversation

kla7
Copy link

@kla7 kla7 commented Jun 14, 2024

Fixes #155 .

@kla7 kla7 requested a review from keighrim June 14, 2024 22:09
Copy link
Member

@keighrim keighrim left a comment

Choose a reason for hiding this comment

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

Some general suggestions;

  • why keeping those first two commits that are simply merging main into the working branch, instead of moving the beginning of the working branch to the latest commit on the main ?
  • the role of app_page_data.py is kind of duplicate of this liquid-based loop, and I wonder if there's an efficient way to de-duplicate those efforts

    apps/docs/index.md

    Lines 17 to 31 in d53ad4e

    {% for app in site.data.app-index %}
    {%- assign check = app[0] | split:'http://apps.clams.ai/' -%}
    {% if check.size == 2 %}
    ### {{ check[1] }}
    {{ app[1]["description"] }}
    {% for version in app[1]["versions"] %}
    * [{{ version[0] }}]({{ check[1] }}/{{ version[0] }}) ([`@{{ version[1] }}`](https://github.com/{{ version[1] }}))
    {% endfor %}
    {% else %}
    ### {{ app[0] }}
    {% for version in app[1] %}
    [{{ version }}]({{ app[0] }}/{{ version }})
    {% endfor %}
    {% endif %}
    {% endfor %}
  • the vXXX part of the breadcrumbs sill doesn't work. I look at the breadcrumb implementation in the theme source, only to find that it's not easy to fix the issue without changing the theme source code (and for that, I forked the theme source to clamsproject org)
  • lastly, I'm thinking of integrating the script in this PR to the website building GH actions script, so that the individual index.md files under app subdirs will be automatically re-generated every time the website is built by the GH. To that end, I don't think we need to include those files in this PR at the end of the day.

import os
import json

directory = '../docs/_apps'
Copy link
Member

Choose a reason for hiding this comment

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

A few issues with this line;

  1. it "hard-codes" '/' as the path separator, and this line will probably break the program on windows.
  2. this assumes the "user" of this script runs the script from REPO_ROOT/scripts directory (i.e., their working directory is REPO_ROOT/scripts) but this isn't always guaranteed or forced. You can make directory paths more "portable" (i.e., resilient to the users' working directory at the runtime) by using __file__ special variable automatically set for every .py file by python interpreter
  3. the variable name directory is too generic. I'd prefer verbose but understandable variable names to concise but almost meaningless names.

So, taking all above in, this line can be, instead, something like this;

apps_dir = os.path.join(os.path.dirname(__file__), '..', 'docs', '_apps')

(as a practice, think about why we need two "go-up" operations (once with dirname, again with '..')

Copy link
Member

Choose a reason for hiding this comment

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

This file seems to use CRLF. Use linux style line breaks. (https://en.wikipedia.org/wiki/Newline)

for app in apps:
app_dir = os.path.join(directory, app)
app_vers = [app_ver for app_ver in os.listdir(app_dir) if os.path.isdir(os.path.join(app_dir, app_ver))]
sorted_app_info = sorted([app_ver.lstrip('v') for app_ver in app_vers], reverse=True) # sort versions from latest to earliest
Copy link
Member

Choose a reason for hiding this comment

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

this will not work, example;

>>> sorted('1.0.1 1.0.2 1.0.10'.split())
['1.0.1', '1.0.10', '1.0.2']

Probably the easiest and safest way to use submission time metadata pulled from each app-version subdir.

for app_info in app_info_dict:
ver_num = app_info_dict[app_info][0]
submitter = app_info_dict[app_info][1]
f.write(f"- [{ver_num}](http://apps.clams.ai/{app}/{ver_num}) ([`{submitter}`](https://github.com/{submitter}))\n")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hard-coding the public website address here (http://apps.clams.ai/), replace with jekyll {{ url }} variable and defer the substitution to jekyll engine. This way the root URL will be automatically adjusted when developing site locally.

@kla7 kla7 force-pushed the 155-app-intermediate-links branch from 6b14d37 to bc443ca Compare June 27, 2024 22:30
@kla7 kla7 requested a review from keighrim June 27, 2024 23:31
@kla7 kla7 force-pushed the 155-app-intermediate-links branch from bc443ca to 1598169 Compare June 28, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Broken links in app pages
2 participants