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

Marketplace should use a file other than README #677

Open
ChristopherRogers1991 opened this issue Nov 24, 2018 · 13 comments
Open

Marketplace should use a file other than README #677

ChristopherRogers1991 opened this issue Nov 24, 2018 · 13 comments

Comments

@ChristopherRogers1991
Copy link
Collaborator

Not sure if this is the correct place to open issues for the marketplace, but I figured it was probably close enough.

This is definitely opinionated, but I don't think a skill's README should be parsed for the Marketplace. I think it would be better to instead use a separate, dedicated file, e.g. a MARKETPLACE.md. By looking for this new file first and falling back to the README if the new file doesn't exist, you could trivially switch to a file like this without breaking existing skills.

There are several reasons I think this should be done. Here are a handful:

  1. The amount of information that can be provided in the README becomes very limited. Because I need to consider the way it renders in the market place, and due to the additional setting in which the information will be displayed, I cannot easily (if at all) include all of the detail I would otherwise. Two specific examples of this:
    1. More than a handful of sample phrases renders poorly - it looks cluttered and jumbled (at least in the preview in the meta-editor - I should note that the meta-editor does not appear to render things correctly in general, so it is possible that this becomes less of an issue in the actual marketplace, though I image there is still an upper limit that is not especially high). My skill has 6 sample phrases in the current README, and it created a pretty overwhelming block of phrases in the preview.
    2. Developer information that would be appropriate for github is less appropriate in the store (more on this below).
  2. The README.md file is already parsed by github (and other git hosting services), and serves as the landing for the repo. Since it already has an explicit purpose, using it for the marketplace as well results in compromises. The most obvious of which is, as noted above, I probably don't want to include developer information in the README any more, as it has no value to normal users.
  3. A dedicated file gives you more flexibility. This ties closely to the above, but when you don't need to allow the file to be github compatible, you can change your format without worrying about a square-peg-round-hole scenario. You could also include details about the marketplace, links to the marketplace and meta-editor, the version of the meta-editor that generated the file, etc - things that would normally clutter the README, but could simply be ignored by the marketplace renderer.

There are other ways to solve the issues above (e.g. I was walking the path of creating a DETAILS.md file that would contain all the info I had to strip out of the README, with links between the two), however, none of them seem quite as appropriate as creating a dedicated file for the marketplace.

This was referenced Nov 24, 2018
@KathyReid
Copy link
Contributor

Thanks so much @ChristopherRogers1991 for your well-considered feedback, as always.

Personally I can seek arguments both for and against having a separate file which is ingested by the Mycroft Marketplace for each Skill. I'm leaning towards not having different files for a few reasons.

  • From a developer experience point of view, we want to make it as easy and painless as possible for Skill Authors to create a new Skill. If we have two different README/Skill files then it's an additional workload for Skill Authors, and more dependencies that have to be maintained between branches of mycroft-skills - this is one of the challenges that we currently have - motivating / encouraging Skill Authors to resubmit Skills to a new branch. Having two files makes this even more challenging, particularly after we changed the format of the README in 18.08.

  • If there are two files to keep updated, they will invariably not be kept in sync, which leads to end user confusion.

  • Having a defined structure for the README serves two purposes - one is so that the README can be ingested by the Marketplace, but the second is that it encourages complete information being provided by the Skill Author - often I will knock back a Skill from mycroft-skills because the README is not structured properly or does not contain sufficient information - I think your Skill documentation is some of the best I've seen - most other Skill Authors are .. shall we say .. at the other end of the documentation spectrum ;-)


The amount of information that can be provided in the README becomes very limited. Because I need to consider the way it renders in the market place, and due to the additional setting in which the information will be displayed, I cannot easily (if at all) include all of the detail I would otherwise.

This is a fair point - the meta-editor as @penrods points out is very much a work in progress at the moment, but I don't think it's a bad preview of how the README will be rendered in the Mycroft Marketplace. In general the Marketplace rendering of README has been fairly good; we've had a couple of edge cases where things like badges don't render particularly well - I think that falls into the category of "developer information that's less relevant for the Marketplace".

One way that we could work around this would be to have another section in the README for badges - and then developer information like badges could go in that section. Do you think this would be workable?

The README.md file is already parsed by github (and other git hosting services), and serves as the landing for the repo. Since it already has an explicit purpose, using it for the marketplace as well results in compromises. The most obvious of which is, as noted above, I probably don't want to include developer information in the README any more, as it has no value to normal users.

Again, I think this could be worked around by having a "developer information" or "badges" section in the README that we could then toggle in the Marketplace. I still think this is preferable to having two different files that the Skill Author needs to maintain.

A dedicated file gives you more flexibility.

True, I agree with you here - but every time we give the Skill Author more to do, we will have fewer Skills submitted to the Marketplace.

I'd be very interested in both @penrods and @chrisveilleux thoughts on this one.

@penrods
Copy link
Contributor

penrods commented Nov 26, 2018

A couple comments:

  • Now badges are legal and can be added to the final README.md
  • Additional sections can be added to the README with dev info, extra instructions, etc.

See: https://github.com/btotharye/mycroft-homeassistant#home-assistant-skill-for-mycroft for an example of a README.md that does this. Also see the Marketplace entry

I don't see the utility of splitting this out into an independent skill, to be honest. This forces devs to be organized in their README and make a good "billboard" for their skill which represents it well if someone visits the repo directly. They also have ample ability to extend it with custom sections.

Perhaps the ability to add additional info is not obvious enough?

@ChristopherRogers1991
Copy link
Collaborator Author

I want to be direct, and I have the suspicion that over a textual communication channel, it's going to come off as very negative and perhaps even hostile, so I want to start by saying that I appreciate you taking the time to follow up with so much detail, and that I appreciate all the work going into the marketplace - I think it's a huge step in the right direction regardless of how this particular issue plays out. Now, keeping that positivity in mind, I want to address your points head on:

From a developer experience point of view, we want to make it as easy and painless as possible for Skill Authors to create a new Skill.

This is already the second time that we've been asked to update READMEs via an automated tool, which makes this a third reworking of my README. This, in my view, is a poor developer experience. The whole reason I write software (and the routine skill is probably the best example of this) is because I don't like doing the same thing over and over again. This may be the last time I'm asked to do this, but the "moving target" comment on #678 makes me think this is unlikely.

If we have two different README/Skill files then it's an additional workload for Skill Authors

The solution I've proposed allows for a second file, but does not require it. It is no additional work for anyone who doesn't want it.

this is one of the challenges that we currently have - motivating / encouraging Skill Authors to resubmit Skills to a new branch.

Part of the problem here is the addition of extra work, e.g. regenerating a README. With a dedicated file, in which I simply enter the exact information you've asked for, and then take the result as is, I would have less work than reworking the README to fit you template, as well as still satisfy my own ideals for what the README should look like on github.

If there are two files to keep updated, they will invariably not be kept in sync, which leads to end user confusion.

This seems like something the review process should catch. This would also only happen in the case where someone has chosen to split the files up, and that, to me, sounds like it would be someone who cares about the documentation enough to either keep it up to date, or not be terribly bothered by being asked to update it if it were caught in review.

it encourages complete information being provided by the Skill Author

Perhaps, but it may also result in the minimum information being provided. I, personally, would not be surprised if people cut things out that don't fit nicely in the template (I've definitely considered it, and may have even cut some out during the last round of README generation).

I think your Skill documentation is some of the best I've seen

Thanks! I do my best.

the meta-editor as @penrods points out is very much a work in progress at the moment, but I don't think it's a bad preview of how the README will be rendered in the Mycroft Marketplace

Here's what I see, if I take as much of my README as I can (which is what sparked all of this):
selection_001

There are many small differences in the CSS, which aren't ideal (I really want to know exactly what it's going to look like), but the markdown doesn't seem to render at all. Plus, from the preview alone, I assumed this was a fixed sized element, and that I needed to cut out a significant amount of information. This is all to say that I disagree, and think the preview leaves a lot to be desired in it's current form (but there's a whole separate issue for that, so no need to go into it more here).

One way that we could work around this would be to have another section in the README for badges - and then developer information like badges could go in that section. Do you think this would be workable?

I'm not sure what you mean by badges? Can you elaborate? From context, I assume this somehow works out to a section that is hidden by default, but could be shown to people that opt in (e.g. by clicking a button or something)?

True, I agree with you here - but every time we give the Skill Author more to do, we will have fewer Skills submitted to the Marketplace.

This follows on from the similar point above, but having skill authors regenerate the README is "more to do." If you can't say, for certain, that this is the absolute last time we'll be asked to regenerate READMEs (and I don't think I'd believe if you did say that), then I assume I'll have to again in the future, and I'm already dreading it.

Additional sections can be added to the README with dev info, extra instructions, etc.

Perhaps the ability to add additional info is not obvious enough?

Tackling the above two together, as they relate - The meta editor says that markdown can be parsed, which alludes to this, but again, if you look at my screenshot above, it renders poorly. Additionally, extra sections with different headers is great exactly until one of them conflicts with a name you'd like to use for additional marketplace info. If we have a name collision, I've now got to deal with coming up with a new one. (Haven't we all spent at least 20 minutes at one point or another trying to come up with the perfect variable name? It's terrible, and this would potentially lead to multiple instances of doing the documentation equivalent. Or worse, as noted above, sections may just be removed, and we end up with minimum documentation.)

Also, if I recall correctly, Joshua mentioned that the Mark II was intended to pass the "grandma test." I think if my grandmother saw the developer notes, and advanced config options, she'd probably back away and assume the skill was too complicated. Putting them in the store puts those details in front of people who have no interest in them, and no understanding of what they mean. Perhaps the badges you've mentioned solves this, but again, you'll have to elaborate, as I'm not sure what you mean.

This forces devs to be organized in their README and make a good "billboard" for their skill which represents it well if someone visits the repo directly.

I don't think this is accurate. The current system, again, forces a minimum. It does not force 'good.' The enforcement of good comes from a review, which is already required any time someone updates their skill (and from what Kathy said, we already know that the skill reviewers are being fantastically vigilant, and bouncing things back that have poor documentation).

I don't know that that will be enough to convince you, but with that, I think I've addressed all of your counter arguments. I want to again say that regardless of the outcome here, while this is frustrating to me, I do think the overall direction is positive, and again appreciate all the work going into this.

@chrisveilleux
Copy link
Member

We appreciate your candor. It's great to get feedback from someone using the tools we are building.

I am going to have to respectfully disagree with my colleagues. A lot of your suggestions are ideas I was in favor of when we designed the Marketplace. I would be interested to know the opinions of other skill developers on this topic.

I'm not sure what you mean by badges? Can you elaborate? From context, I assume this somehow works out to a section that is hidden by default, but could be shown to people that opt in (e.g. by clicking a button or something)?

I believe the badges Steve is referencing are things like "tests passing", "coverage 99%", etc. See the home assistant skill for examples.

Also, if I recall correctly, Joshua mentioned that the Mark II was intended to pass the "grandma test." I think if my grandmother saw the developer notes, and advanced config options, she'd probably back away and assume the skill was too complicated. Putting them in the store puts those details in front of people who have no interest in them, and no understanding of what they mean. Perhaps the badges you've mentioned solves this, but again, you'll have to elaborate, as I'm not sure what you mean.

I think this is a great point. The message you want to convey to developers looking at your repo may be different than the message you want to convey to consumers.

I would love to build a tool that allowed users to edit their Marketplace content for each skill. A tool that saves the content, allowing authors to tweak values rather than start over each time. A tool with a marketplace preview that potentially re-used UI code from the Marketplace. However, this is a lot of work that would need to be prioritized appropriately.

The solution I've proposed allows for a second file, but does not require it. It is no additional work for anyone who doesn't want it.

I agree with this point. Why not give authors the option to present their skill differently on the Marketplace if they so choose? Again, I would be curious to get more input from other skill developers to know how many would take advantage of such an option. We talk all the time in UX meetings about not making decisions for our users; giving them a voice in how we design our applications. Why is this any different?

@KathyReid
Copy link
Contributor

Hi @ChristopherRogers1991, in the interests of total openness, I wanted to let you know that we discussed this at Skills Team Meeting last fortnight. I will keep you updated on progress.

Best, Kathy


Feedback on the Meta Editor from Christopher Rogers

#677
#678
Should we have a separate file for Skill Authors for the Marketplace and for the GitHub README.md?

DISCUSSION: The end goal is to have the preview be an accurate preview; we’re not quite there. If we were to have a second file, then JSON is easier to parse than Markdown. Hand editing JSON is painful; escaping special characters like quotes is difficult.
DECISION: We don’t want two Markdown files.
DISCUSSION: Is it worthwhile to have the option of either one? This is doable, but is still a significant amount of work. The Meta Editor could output to JSON instead of the Markdown format. We have mixed feelings on this. Pixel perfect preview is hard to maintain; it it sets a high expectation - we want the expectation to be that the preview is an approximation, not pixel perfect. We are happy to go back to the tool to make it a closer preview - we are even happier if someone else in the Community wishes to assist with this - it’s straightforward HTML and CSS.
ACTION: Kathy to put a call out for Community assistance to help make the preview on the Meta Editor more realistic. Some information might be needed from Chris or Davit to get the CSS files.

The Harvester could look to see if there’s a marketplace.json - the infor could be pulled out of that instead. None of this work is difficult - but it can be time consuming and the immediate benefits are not clear.

DISCUSSION: Is there a service that shows package equivalencies across different distros? That would make requirements.sh a lot easier to manage - at the moment the Skill Author has to manually manage dependencies for different distros. Is there something we can do to make managing dependencies easier for the Skill Author, so that they don’t have to manage dependencies themselves?

There is something called AppStream which may be available, but it’s not ready yet:
https://en.wikipedia.org/wiki/AppStream

DISCUSSION: Kathy this we need to be clearer about cross-distribution support. For example, we support Ubuntu/debian based distributions / few others. It then becomes the responsibility of the other distributions to help package Mycroft for their distribution. This doesn’t solve the problem of individual Skills - ie the individual Skills would have to support the distribution - ie via requirements.sh. For example, the protobuf issue on AIY because Google released a new package.

ACTION: Kathy to add this to the Support Strategy work that is upcoming.

DISCUSSION: How do we get people to “share back” once they have got something running on their distribution? How do we encourage that sort of behaviour? Is GitHub too intimidating?

ACTION: Additional content in an extra field - we need to market / make Skill Authors more aware of this
ACTION: Add a freeform field then Kathy to add something to the newsletter to raise awareness of this.
ACTION: update the preview to be closer
Then we can consider larger tranches of work in the longer term

DISCUSSION: GitHub is not end user friendly to people who are not familiar with GitHub; most of our “edge case distro” users are likely to know GitHub. We don’t want to move away from GitHub, but can we make it easier to use? This might be a pushing upstream issue - they share the solution in the Forums, but don’t want to sign the CLA or raise the PR to merge upstream. Why is the CLA a barrier? Doesn’t feel “open source” - ie many open source projects don’t have a CLA - I just submit a pull request. A lot of thought went into the CLA - we can’t really do without it on the project.

IDEA: Make it clearer that you don’t have to sign a CLA to raise an issue on a repo, only if you are submitting a PR to repos that are licensed Apache 2. We don’t know the size of the problem. People are discussing it in distro forums, not the Mycroft community. When the license changed from GPL to Apache2, there wasn’t a lot of pushback at all.

IDEA: Could we do a blog post about why we have a CLA to try and shake out concerns from the community around why they may not want to sign a CLA - a bit of background around why Apache 2, and how the CLA is part of the Apache 2 license implementation. Ie lay out the bona fides - we’ve been really honest and ethical about this.

ACTION: blog post about Apache 2 / CLA / open discussion / open forum post

@ChristopherRogers1991
Copy link
Collaborator Author

Hey Kathy,

Thanks for the update and the details. A couple thoughts:

Pixel perfect preview is hard to maintain; it it sets a high expectation - we want the expectation to be that the preview is an approximation, not pixel perfect.

Is there a particular reason it can't be the same code generating both? E.g. the same JavaScript, HTML, CSS resources shared between the two? Alternatively, the marketplace could have a 'preview' endpoint that accepts the data it needs in a POST body, and renders using the actual production assets.

These are just a couple half-thoughts, but I think it might be worthwhile to reframe the goal as 'write once, render everywhere,' rather than trying to approximate, and keep the preview up to date with the store. (You may have already considered this, so apologies if there's something I'm missing here that would make this impossible).

The end goal is to have the preview be an accurate preview

Just to clarify on this - my main frustration here is actually reworking my README multiple times. The preview not being accurate just made me think this was going to be more work than it actually should have been. Based on the preview, I assumed the card was a fixed size, the markdown wouldn't render the way I wanted it to, etc, and began making significant changes to my README. As I mentioned on #678, I'd actually be ok with the preview being turned off, and just given links to examples. In that case though, or even with a perfect preview of my file, I still think there is merit to having two files.

@LinusSkucas
Copy link
Contributor

Hey, just adding in to this, I like the idea of 2 files, and with the new manifest.yml file, the manifest.yml could be updated to include what the meta editor generates (icon, description etc....). If developers wanted to use the readme file, then they could, but if they wanted more flexibility with the readme, then they could use the manifest.yml file.

Personally, for all the skill that need configuring, I use GitHub wikis.

@penrods
Copy link
Contributor

penrods commented Mar 11, 2019

The idea of extending the Manifest seems like a good fit. @ChristopherRogers1991, what do you think of that proposal?

I'd ideally have a version # and include a language identifier so we can allow localized versions to be added for future localized Marketplace implementations and the Poodle translation tool.

@krisgesling
Copy link
Contributor

Hey was just looking at this and wondering if there's any benefit in listing supporting languages in the manifest, or if that's just an extra thing to update and we should just consider a language supported if the relevant folders exist in all the right places eg locale/es-es?

@andlo
Copy link
Contributor

andlo commented May 8, 2019

One other aspect is which language is suited for translation the skill.

If one makes a skills in en, and uses some API that also can return in de and fr, then the skil is made in en, and is suitable for translating to de and fr. But dosnt make sense to translate to sv and dk as the api dosnt return in those languages.

And therefore it isnt ideal that such a skill pops up on translate.mycroft.ai in other languages than the devloper mark the skills suatible to be translated to.

So there are to language specifikations - one which language are current supportet and can be seen in the lang folder on the skill, and one for langiuage that could be supported if skill was translated

@LinusSkucas
Copy link
Contributor

Listing the languages would be cool, but it would be nice, if a skill doesn't have any language requirements, to have the translate export tool update it when all of the language for that skill is done.

@penrods
Copy link
Contributor

penrods commented May 8, 2019

At the moment, I think we just stick with the presence of a translation folder, @krisgesling . I know many translations are only partial, but we don't have any way currently to determine when a translation is "done". We'd need native speakers to validate that for us, and there isn't that structure yet.

@krisgesling
Copy link
Contributor

Yeah there's some scenarios like andlo has outlined that may warrant this in the future but probably safer to leave it out until we're sure about the implementation.

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

No branches or pull requests

7 participants