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

Add extension to support metaformats #213

Merged
merged 11 commits into from Dec 1, 2023

Conversation

snarfed
Copy link
Member

@snarfed snarfed commented Nov 28, 2023

implements all of https://microformats.org/wiki/metaformats except explicit microformats2 classes on <meta> tags. also falls back to inferring name from <title> and summary from meta[name=description], neither of which is (yet) part of the official metaformats spec.

Copy link
Collaborator

@angelogladding angelogladding left a comment

Choose a reason for hiding this comment

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

Looks good. I've fixed the whitespace/documentation in CHANGELOG/CONTRIBUTING upstream to clean up this PR. And this will be a part of 2.0 instead of 1.2 as noted, correct?

@snarfed
Copy link
Member Author

snarfed commented Nov 29, 2023

Thanks for reviewing! And I have no idea what the next mf2py version should be, I'll happily defer to you and @capjamesg etc. Afaik this has no breaking changes at least.

@sknebel
Copy link
Member

sknebel commented Nov 29, 2023

I think it wold make sense for metaformats to be an external extension at this point, since I kind of expect that people would want/need to experiment with variations of it instead of relying on it being updated with mf2py.

But as long as it's behind a flag I'm also not strictly against including it.

@snarfed
Copy link
Member Author

snarfed commented Nov 29, 2023

I think it wold make sense for metaformats to be an external extension at this point, since I kind of expect that people would want/need to experiment with variations of it instead of relying on it being updated with mf2py.

Interesting! By "external extension," I assume you mean code that's entirely outside mf2py, not some plugin capability that would hook into mf2py, right?

snarfed added a commit to snarfed/bridgy-fed that referenced this pull request Nov 29, 2023
...temporarily, until microformats/mf2py#213 is merged and released
@snarfed
Copy link
Member Author

snarfed commented Nov 29, 2023

Looks like we have a +0 or +1 on this from @angelogladding, a -0 from @sknebel (I think), and a +0 from me. @capjamesg any opinion on whether we merge or reject this PR?

@snarfed snarfed mentioned this pull request Nov 30, 2023
@sknebel
Copy link
Member

sknebel commented Nov 30, 2023

Interesting! By "external extension," I assume you mean code that's entirely outside mf2py, not some plugin capability that would hook into mf2py, right?

It'd make sense for it to hook into mf2py, for easy of use and to avoid parsing things twice etc.

If needed we could add an interface for that, but from a quick look at your implementation I think you could do it by deriving your own class from Parser that overrides parse_el with a function that calls the original and then your metaformats parsing function? (EDIT: a package doing that could thus iterate independently of mf2py)

@capjamesg
Copy link
Member

I agree with @sknebel's note above.

@snarfed
Copy link
Member Author

snarfed commented Nov 30, 2023

OK! Sounds good. Feel free to go ahead with 2.0!

@snarfed snarfed closed this Nov 30, 2023
@capjamesg
Copy link
Member

@snarfed You can keep this open if you would like!

@snarfed
Copy link
Member Author

snarfed commented Nov 30, 2023

I'm ok either way! Up to you all.

@capjamesg
Copy link
Member

@snarfed Angelo and I spoke about this on a call and we'd like to merge this into the next release. We can always implement an extensions API later with backcompat, but for now this code solves a problem and does so efficiently and elegantly.

@capjamesg capjamesg reopened this Nov 30, 2023
snarfed added a commit to snarfed/webutil that referenced this pull request Nov 30, 2023
@angelogladding angelogladding changed the title first pass at implementing metaformats Add extension to support metaformats Dec 1, 2023
@angelogladding angelogladding merged commit 698f2bb into microformats:main Dec 1, 2023
@capjamesg
Copy link
Member

Reading over this again, @sknebel's suggestion wasn't explicitly for an extensions API. @sknebel, thank you for encouraging me to revisit this. My comment from yesterday was conflation on my part from another related discussion Angelo and I were having re: how we structure additional functionality that is not explicitly microformats, as well as what we think about as an "extension" right now (image alt text being the only implementation at present).

I understand there are situations where someone would only want a metaformats parser. The integration in the microformats parser feels right given that metaformats are in discussion by the microformats community. Given the work already done by @snarfed, and its elegant -- and non-intrusive -- usage of the current Parser class, it felt prudent to accept this PR as is.

@angelogladding
Copy link
Collaborator

angelogladding commented Dec 4, 2023

I believe developers that want the <META> items in mf2json format also want the mf2s on the same page. Those who don't can obviously ignore the normal mf2 items. This works trivially if we stuff a top level meta-item property (vs a source=metaformats property on the last item of items) -- see microformats/metaformats#2 (comment) and idea no. 3 in #197 (comment) with which I agree.

I believe the functionality should wind up here eventually so I'd rather skip the overhead of a second project if possible. @sknebel do you foresee there being too many changes up front or all throughout the lifetime of the metaformats spec?

We've already committed to release more frequently. Remember, the road to mf2py 2.0 started with a call to "modernize". It has included deprecating py2, improving the project tooling, cleaning up old issues and implementing modern mf2 features. 2.1 will be more focused and lightweight. And we shouldn't hesitate to shoot to 3.0 from there if we need (as I believe we've already discussed). In that case this project should be well suited to absorb the initial high rate of change that metaformats inclusion may bring.

From a branding perspective mf2py can rebrand from "MicroFormats2 for PYthon" to "[Micro|Meta]Formats 2 PYthon" without telling anyone.

@sknebel
Copy link
Member

sknebel commented Dec 4, 2023

Having it externally also makes it somewhat easier for users to experiment by changing only the metaformats parser while not directly touching mf2py, and to me metaformats seems very much at a stage where I expect quick experimentation will be important to figure out what works and what doesn't (and I think that there is not even agreement how the metaformats result should be added to the output is showing that too), and that's ideally without mf2py releases in the loop even if we do more of them (which we should!).

I think that for this stage the metaformats parsing doesn't have to be much of a "project" with structured releases etc, and thus the cost of that is low, I was more thinking along the lines of "single file in a repo people copy and tweak".

But that's merely a preference/assumptions of mine, having it behind a feature flag and iterating as needed inside mf2py is ok too if that's what others prefer. (Really my main annoyance with the discussions has been that it repeatedly sounded like "@sknebel wants to add an extension API" when my point was that we do not need one and the parser can already be expanded as needed)

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.

None yet

4 participants