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

Support additional metadata #44

Merged
merged 7 commits into from
Apr 28, 2023

Conversation

ES-Alexander
Copy link
Collaborator

@ES-Alexander ES-Alexander commented Apr 25, 2023

  • adds backwards-compatible support for the new LABELs recommended in our docs
  • sorts the versions, to ensure the latest (valid) version is first in the manifest (which can then be used for extracting the type / tags for filtering and sorting)
    • the previous order was up to how dockerhub provides the tags, which is most likely "latest first", which is generally what we want but would fail if multiple major versions are being maintained simultaneously, and the old major version has a more recent update than the current major version
  • updates the website to
    1. display the tags from the latest version of each extension
      • intentionally made to look kind of bad if there are no tags, as motivation to add tags
    2. section the extensions by their type
      • no extensions use this field yet, so they all default to other
        • I'll submit some PRs to set reasonable types for the existing extensions, although it will take until their next semver release to be noticed by the website
    • more advanced approaches would allow dynamic sorting, and filtering by the tags, but this is a decent start
    Screenshot 2023-04-28 at 11 44 42 pm

relevant to #34

@ES-Alexander ES-Alexander force-pushed the tags-type branch 4 times, most recently from bdd1f12 to 5f4c6a6 Compare April 25, 2023 11:25
@ES-Alexander
Copy link
Collaborator Author

I have no idea how to declare the types in the build check. Is there some way to just ignore those checks? Actually building the site works fine.

@patrickelectric
Copy link
Member

just ignore those checks

image

You can add a computed property that will return the correct type.

blueos_repository/consolidate.py Outdated Show resolved Hide resolved
blueos_repository/consolidate.py Outdated Show resolved Hide resolved
blueos_repository/consolidate.py Outdated Show resolved Hide resolved
@ES-Alexander
Copy link
Collaborator Author

ES-Alexander commented Apr 25, 2023

You can add a computed property that will return the correct type.

@patrickelectric Not sure how to do that - it took me quite a while to even figure out how to make the logic work.

blueos_repository/consolidate.py Outdated Show resolved Hide resolved
blueos_repository/consolidate.py Show resolved Hide resolved
blueos_repository/consolidate.py Outdated Show resolved Hide resolved
@patrickelectric
Copy link
Member

patrickelectric commented Apr 26, 2023

You can add a computed property that will return the correct type.

@patrickelectric Not sure how to do that - it took me quite a while to even figure out how to make the logic work.

You should do something like this:

<template>
  <v-container>
    <v-row
      v-for="(section, index) in sections"
      :key="index"
    >
      <h1>{{ section }}</h1>
      <v-row>
        <v-col v-for="extension in extensions" :key="extension.identifier">
          <v-card v-if="latestVersion(extension).type === section" class="mx-auto" width="400px" outlined>
            <v-card-title>
              <v-row no-gutters class="justify-center align-center">
                <v-avatar tile size="50">
                  <v-img :src="extension.extension_logo" />
                </v-avatar>
                <v-col>
                  <v-card-title>
                    <a v-bind:href="extension.website">{{ extension.name }}</a>
                  </v-card-title>
                  <v-card-subtitle>
                    <a v-bind:href="'https://hub.docker.com/r/' + extension.docker">{{ extension.identifier }}</a>
                  </v-card-subtitle>
                  <v-card-actions>
                    <v-chip label class="ma-1" color="blue" v-for="tag in latestVersion(extension).filter_tags">
                      {{ tag }}
                    </v-chip>
                  </v-card-actions>
                </v-col>
              </v-row>
            </v-card-title>
            <v-card-text>{{ extension.description }}</v-card-text>
          </v-card>
        </v-col>
      </v-row>
    </v-row>
  </v-container>
</template>

<script setup lang="ts">
import { computed, onMounted, ref } from "vue";
import { ExtensionType, RepositoryEntry, Version } from "@/types/manifest"

const extensions = ref();

onMounted(async () => {
  const response = await fetch("manifest.json");
  extensions.value = await response.json() as [RepositoryEntry];
});

const sections = computed(() => {
  return ExtensionType
})

function latestVersion(extension: RepositoryEntry): Version {
  const versions = Object.keys(extension.versions)
  // Use semver to sort versions
  return extension.versions[0 /* use latest version from sort*/]
}
</script>

@ES-Alexander
Copy link
Collaborator Author

function latestVersion(extension: RepositoryEntry): Version {
  const versions = Object.keys(extension.versions)
  // Use semver to sort versions
  return extension.versions[0 /* use latest version from sort*/]
}

Hah! That's basically already what I ended up doing :-)

Is the const there important? I just did a single line return statement.
Also, on the sorting front, I made the consolidate.py file sort the versions before putting each package in the manifest, so they should already be in the desired order, right?

@patrickelectric
Copy link
Member

function latestVersion(extension: RepositoryEntry): Version {
  const versions = Object.keys(extension.versions)
  // Use semver to sort versions
  return extension.versions[0 /* use latest version from sort*/]
}

Hah! That's basically already what I ended up doing :-)

Is the const there important? I just did a single line return statement. Also, on the sorting front, I made the consolidate.py file sort the versions before putting each package in the manifest, so they should already be in the desired order, right?

In theory yes, but you should not trust that you are going to receive it sorted in future versions.

@patrickelectric
Copy link
Member

patrickelectric commented Apr 27, 2023

function latestVersion(extension: RepositoryEntry): Version {
  const versions = Object.keys(extension.versions)
  // Use semver to sort versions
  return extension.versions[0 /* use latest version from sort*/]
}

Hah! That's basically already what I ended up doing :-)

Is the const there important? I just did a single line return statement. Also, on the sorting front, I made the consolidate.py file sort the versions before putting each package in the manifest, so they should already be in the desired order, right?

Something that I forgot to mention, with #45, now we can have safe types, and the computed method return a know type and with this the typescript gets validated.

@ES-Alexander ES-Alexander force-pushed the tags-type branch 2 times, most recently from 5e1c4d1 to e01aa74 Compare April 28, 2023 06:32
@ES-Alexander ES-Alexander force-pushed the tags-type branch 3 times, most recently from 47fba5c to 1d89d8e Compare April 28, 2023 13:45
@ES-Alexander
Copy link
Collaborator Author

ES-Alexander commented Apr 28, 2023

Spent a bunch of time fixing the appearance. It's still not stunning, but it's much better than it was, and there are no longer several invisible elements making the spacing all weird :-)

Comment on lines +16 to +20
class StrEnum(str, Enum):
"""Temporary filler until Python 3.11 available."""

def __str__(self) -> str:
return self.value # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

class HumanStringEnum(str, Enum):
    def __str__(self) -> str:
        return self.name.lower().replace('_', '-')

Comment on lines +57 to +61
class ExtensionType(StrEnum):
DEVICE_INTEGRATION = "device-integration"
EXAMPLE = "example"
THEME = "theme"
OTHER = "other"
Copy link
Member

Choose a reason for hiding this comment

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

class ExtensionType(HumanStringEnum):
    DEVICE_INTEGRATION = auto()
    EXAMPLE = auto()
    THEME = auto()
    OTHER = auto()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't create desirable values.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

@ES-Alexander ES-Alexander Apr 28, 2023

Choose a reason for hiding this comment

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

Yes, it prints nice things, but the actual values end up as string versions of numbers.

>>> ExtensionType.DEVICE_INTEGRATION
<ExtensionType.DEVICE_INTEGRATION: '1'>
>>> ExtensionType.DEVICE_INTEGRATION == 'device-integration'
False
>>> ExtensionType.DEVICE_INTEGRATION == '1'
True

blueos_repository/consolidate.py Show resolved Hide resolved
Comment on lines +81 to +83
def validate_filter_tags(tags: List[str]) -> List[str]:
"""Returns a list of up to 10 lower-case alpha-numeric tags (dashes allowed)."""
return [tag.lower() for tag in tags if tag.replace("-", "").isalnum()][:10]
Copy link
Member

Choose a reason for hiding this comment

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

We should enforce our defined tags and not allow custom ones.

return [tag for tag in tags if tag in (str(e) for e in ExtensionTags)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have fixed tags, just fixed types. If someone wants to create an arbitrary tag then they can.

Copy link
Member

Choose a reason for hiding this comment

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

So... I can have a tag that will show on the interface as Potato, witchcraft, emojis, non latin characters and etc ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, they're required to be lower-case alpha-numeric tags (with dashes allowed), and there's a limit of 10 per extension, as specified in the docstring of that function, as well as in our extensions metadata documentation and in the readme as well.

authors = raw_labels.get("authors", None)
docs = raw_labels.get("docs", None)
docs = links.pop("docs", links.pop("documentation", raw_labels.get("docs", None)))
Copy link
Member

Choose a reason for hiding this comment

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

why the main "docs" labels is not the priority ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Documentation is just a URL, so it should go as an entry within the new "links" label. If links does not contain "docs" or "documentation" then there's a fallback check for the old structure that had "docs" as an independent label.

Copy link
Member

Choose a reason for hiding this comment

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

We should add something on CI to avoid people using the deprecated docs from the old structure. That can be done in future PRs, but we should at least create an issue to keep a note.
Besides that, can you add a comment on the code explaining it as well ?
There is no comments on code and in the commit about this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should add something on CI to avoid people using the deprecated docs from the old structure. That can be done in future PRs, but we should at least create an issue to keep a note.

I've added a note to #30

Besides that, can you add a comment on the code explaining it as well ?

Done

There is no comments on code and in the commit about this change.

I was originally considering the point about 'links' in the commit description to be sufficient, since I don't think this is a particularly consequential or important change - the field isn't currently being used in BlueOS, and most existing extensions have no value for it.

- `company` is a json, which currently only contains an "about" section for a brief description about your company.
- `company` is a json with information about the maintainer responsible for providing new versions
- `readme` is a URL to a markdown-based README file for the extension
- `links` is a collection of additional useful/relevant links
Copy link
Member

Choose a reason for hiding this comment

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

it does not say about the special case for docs and documentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that's particularly important to specify - the "links" label in general can contain arbitrary links that are decided on by the user, we'll most likely just be displaying them all as hyperlinks.

@patrickelectric patrickelectric merged commit 7152c0b into bluerobotics:master Apr 28, 2023
@ES-Alexander ES-Alexander deleted the tags-type branch April 28, 2023 16:34
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