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

Allow dots in application name (fix #1525) #888

Open
wants to merge 1 commit into
base: stretch-unstable
from

Conversation

@southerntofu
Copy link

southerntofu commented Mar 12, 2020

The problem

Described in #1525

Solution

Add a literal dot in the regex to validate application names.

PR Status

Tested by @Gofannon. Seems to fix the issue for them.

Doctests were not run due to my failure at setting up the dev environment. I'll come back to you when i succeed

How to test

Installing and removing an app with a dot in the name should not fail. This can be tried with the diagrams.net application.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 14, 2020

Hello, as we have decided to use the dot to separate the permission name and the application in own permissions system I don't think that it's really a good idea to accept this.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Mar 14, 2020

Sounds to me that the problem isn't that dots should be allowed, but on the contrary that dots shouldn't be allowed during the installation.
https://github.com/YunoHost/doc/blob/master/packaging_apps_guidelines.md#yep-11

@Gofannon

This comment has been minimized.

Copy link

Gofannon commented Mar 17, 2020

Sounds to me that the problem isn't that dots should be allowed, but on the contrary that dots shouldn't be allowed during the installation.
https://github.com/YunoHost/doc/blob/master/packaging_apps_guidelines.md#yep-11

Agreed. It should be forbidden in the first place.
I didn't run package_check to verify the compliance against YEP rules. I installed it straight on YunoHost server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.