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

[Submit add-on]: AI Content Describer 2023.11.21 #2037

Closed
cartertemm opened this issue Nov 21, 2023 · 8 comments
Closed

[Submit add-on]: AI Content Describer 2023.11.21 #2037

cartertemm opened this issue Nov 21, 2023 · 8 comments
Labels
autoSubmissionFromIssue Automatic add-on submission from issue template. Triggers an auto-merge PR.

Comments

@cartertemm
Copy link

Download URL

https://github.com/cartertemm/AI-content-describer/releases/download/v2023.11.21/AI.Content.Describer-2023.11.21.nvda-addon

Source URL

https://github.com/cartertemm/AI-content-describer/

Publisher

Carter Temm

Channel

stable

License Name

GPL v2

License URL

https://www.gnu.org/licenses/gpl-2.0.html

@cartertemm cartertemm added the autoSubmissionFromIssue Automatic add-on submission from issue template. Triggers an auto-merge PR. label Nov 21, 2023
Copy link
Contributor

Welcome to the add-on store submission process.
As this is your first submission for AI Content Describer, you will need manual approval as a submitter.
If you are not the owner of the main repository for this add-on, please provide evidence that you have permission to submit this add-on.
Please wait until # is merged.

@cartertemm
Copy link
Author

Hello @seanbudd,

I'm attempting to figure out why the associated Github action is failing to create a PR, as it's seemingly choking on the verifySubmitter workflow.

FWIW this is my first time submitting to the store. My understanding was that I simply had to fill out the issue template and the rest would be handled by the automated workflows. Am I missing something?

@seanbudd
Copy link
Member

Hi @cartertemm - this is failing as the "name" field in your manifest contains spaces. This is not expected, and we should add validation to check for this in NVDA and in the submission process.
Please resubmit with a manifest using the name "AIContentDescriber" instead

@seanbudd seanbudd closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2023
@lukaszgo1
Copy link

this is failing as the "name" field in your manifest contains spaces. This is not expected, and we should add validation to check for this in NVDA and in the submission process. Please resubmit with a manifest using the name "AIContentDescriber" instead

@seanbudd One of my add-ons (which I haven't yet submitted into the store) would also be affected by this i.e. name contains spaces. Note that this was not a problem until now both in therms of accessing it from the website, and in terms of updating via Add-on Updater. While for the new add-on like the one @cartertemm submitted renaming is not much of a problem, for existing ones there are at least two difficulties with doing so:

  1. The new version of the add-on won't be shown as an upgrade for existing users
  2. After installation user ends up with the two add-ons doing the same thing, differing just by the space in its name. I can certainly introduce some code in install tasks to uninstall the old version if necessary, but that is an additional code I have to write

Can we consider lifting the no spaces in the identifier restriction for the store? How difficult it would be to implement? I may look into this assuming the general direction is okay with you.

@nvdaes
Copy link
Sponsor Contributor

nvdaes commented Nov 22, 2023

Hello. If apaces are accepted in add-on ids, I think that compatibility with the translation system should be considered too.
In the guidelines writen by @mhameed, creator of the system used for add-on (and previously NVDA) translation, we can read:

  1. Addon name should be of the form "name", or "firstSecond" or "first_second"
    examples: "word", "dropbox", "extendedWinamp", "resourceMonitor" or "resource_monitor". Dashes in names are currently not supported by the automated system.

@lukaszgo1
Copy link

@nvdaes My understanding is that we would need to invent some system for add-ons translation, since the one used currently is going to be decommissioned at some not too distant point. When creating whatever system comes next we would need to take this into account (among with different oddities of the old system i.e. forcing users to make their repository match name in the manifest should be no longer necessary for example), but this should be discussed separately.

@nvdaes
Copy link
Sponsor Contributor

nvdaes commented Nov 22, 2023

My understanding is that we would need to invent some system for add-ons translation, since the one used currently is going to be decommissioned at some not too distant point. When creating whatever system comes next we would need to take this into account (among with different oddities of the old system i.e. forcing users to make their repository match name in the manifest should be no longer necessary for example), but this should be discussed separately.

I'm not sure about this. If the system is not decommisioned, I think it makes sense to bring this here. Inventing a new system is not easy and in the way many add-ons may not be translated.
Deppending on plans about the add-ons translation system, maybe useful consider this problem in the same issue, for example if documentation needs to be added, or even to prioritize the work on the issue about spaces at this moment.

@seanbudd
Copy link
Member

@nvdaes @lukaszgo1 - You're right, this is a regression from when we introduced submission verification.

Fixed in #2057

seanbudd added a commit that referenced this issue Nov 26, 2023
…#2057)

A recent regression meant that add-on IDs containing spaces caused the verify submitter PR to fail when an add-on is submitted for the first time.

This can be fixed by removing special characters when creating the temp branch for the PR to add the author as an approved submitter.

Tested with this add-on:
nvaccess#54

Example failure on production:
#2037
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoSubmissionFromIssue Automatic add-on submission from issue template. Triggers an auto-merge PR.
Projects
None yet
Development

No branches or pull requests

4 participants