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

fix(contentlayer): Improve Topics Processing and Generation #61

Merged

Conversation

kouloumos
Copy link
Member

This PR fixes #54 and and introduces improvements to the structure and handling of topic data in the content layer.

Key Changes

  • Simplify topics data generation while resolving the inconsistencies in topic names on the /categories and /topics pages.
  • Transitioned to using our own topics index, replacing reliance on Optech's.
  • Renamed JSON outputs for better understanding:
    • tag-data.jsontopics-by-category-counts.json
    • topics-data.jsontopics-counts.json
  • Added enhanced display options for miscellaneous topics.

Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
registry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2024 3:20pm

contentlayer.config.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@IgboPharaoh IgboPharaoh left a comment

Choose a reason for hiding this comment

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

Great work on the pr and addition of our own topic index. That said, I've been able to test the pr both the latest deployment and the code and I think everything works accordingly. I left a comment on what I think might be a value add but everything works as is.

- fix issues with generation of topics
- streamline the code by grouping the calculations and generation into a single method
- more meaningful names for json outputs
@kouloumos
Copy link
Member Author

Rebased and made a minor change based on @IgboPharaoh comment.

@Emmanuel-Develops
Copy link
Collaborator

A couple of comments I made elsewhere, providing them here for context:

the tags on bitcointranscripts do not always correlate to the topics in https://github.com/bitcoinsearch/topics-index/blob/main/topics.json

For example, this transcript
has tags: lightning and p2p the corresponding topics in topics index are P2P Network Protocol

These tags aren't topics in the topics.json from the topic index, neither are they any topic aliases. Therefore they won't be accounted for in category page, in this example P2P Network Protocol

@kouloumos what is the consensus whether the changes will be on bitcoin-transcript or we add them to the respective alias in topics index?

Also this change directly maps to how we select topics in transcripts queuer (switch to utilizing topic index rather than optech tags)

@kouloumos
Copy link
Member Author

In the /categories page they are currently under "Miscellaneous" as they were before this PR. So, we could merge this and care about handling them later.

That said, I just added a configuration in the topics-index that allows for custom category slugs that solves this issue. So we should be good to go!

To give additional context on your questions:

or we add them to the respective alias in topics index?

What you are describing is not how we currently use the aliases field in the topics index. The aliases as taken from their usage in the Optech Index are used for other known names of a topic. For example "Scriptless scripts" for "Adaptor Signatures". So, it doesn't make sense to add p2p or lighting as aliases of the respective Misc topic tags that we are creating for each category.

The issue arises from the fact that those specific tags/slugs don't follow the pattern that we have with the rest of the misc category tags.

Also this change directly maps to how we select topics in transcripts queuer (switch to utilizing topic index rather than optech tags)

good point! We will need to change that also. It will be a simple change, just switching the source of truth JSON. I don't really like the way that we are currently selecting those topics inside the Editor, as we are only using the slugs, with no additional context to help the reviewer decide/understand the tag. We have that information (through the excerpt) so I'll open an issue with clarifications on the improvement that we can do there. That change could be combined with the switch to our own topics-index.

…ology

- terminology change to refer to "topics" instead of "categories"
- Simplify data processing to do a single pass and use a single structure
- Consolidate JSON generation into cleaner functions
- start using our own topics index instead of optech's
- add different display option for misc topics in categories
@Emmanuel-Develops
Copy link
Collaborator

In the /categories page they are currently under "Miscellaneous" as they were before this PR. So, we could merge this and care about handling them later.

They aren't under miscellaneous anymore in the latest deployment.
here, doesn't have the p2p and lightning tags/topics in miscellaneous which are not accounted for in other categories.
cc: @kouloumos

This PR looks good to go apart from that

@kouloumos
Copy link
Member Author

kouloumos commented Dec 4, 2024

They aren't under miscellaneous anymore in the latest deployment.

They were at the time of your comment. Then I pushed a change (which I am explaining in the same comment) to deal with the issue that you raised.

So now, you can see a "Miscellaneous" topic under the "P2P Network Protocol" and "Lightning Network" categories, following the same pattern as the rest of the categories.

@Emmanuel-Develops
Copy link
Collaborator

They aren't under miscellaneous anymore in the latest deployment.

They were at the time of your comment. Then I pushed a change (which I am explaining in the same comment) to deal with the issue that you raised.

So now, you can see a "Miscellaneous" topic under the "P2P Network Protocol" and "Lightning Network" categories, following the same pattern as the rest of the categories.

okay got it

@Emmanuel-Develops Emmanuel-Develops merged commit 5907312 into bitcointranscripts:staging Dec 4, 2024
2 checks passed
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.

3 participants