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 collaboration type #521

Merged
merged 3 commits into from
Sep 27, 2017
Merged

add collaboration type #521

merged 3 commits into from
Sep 27, 2017

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Sep 18, 2017

Depends on nextcloud/server#6328, used in nextcloud/circles#126

It looks fairly broad and complex… because "collaboration" is a pretty wide term and I've no idea what else could come there in future (or not).

shareType (line 461) could be an enumeration as well, however it would mean naming constants here and have a coupling to code. Not sure this is necessary, but if worthwhile I can change this. Opinions welcome about it.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@coveralls
Copy link

coveralls commented Sep 18, 2017

Coverage Status

Coverage increased (+0.002%) to 97.545% when pulling 05d1393 on add-collaboration into 87c3421 on master.

@BernhardPosselt
Copy link
Member

BernhardPosselt commented Sep 19, 2017

Needs a description of what the tag actually does here in the docs: http://nextcloudappstore.readthedocs.io/en/latest/developer.html#info-xml

See https://github.com/nextcloud/appstore/blob/master/docs/developer.rst

As for the structure: the nesting seems a bit excessive given that you only want to be able to register search plugins. Maybe its possible to bring it down to something like (just some playing around, do not know the usecase enough for that):

<collaboration>
    <plugin type="search" sharing="SHARE_TYPE_CIRCLE">OCA\Circles\Collaboration\v1\CollaboratorSearchPlugin</plugin>
</collaboration>

As for the share type: is there a finite number of types? If so, use an enumeration.

also add some doc

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented Sep 26, 2017

@BernhardPosselt ty! Your remarks are addressed

@coveralls
Copy link

coveralls commented Sep 26, 2017

Coverage Status

Coverage decreased (-0.03%) to 97.516% when pulling a094d19 on add-collaboration into 87c3421 on master.

@BernhardPosselt
Copy link
Member

Very small nitpick: XML and HTML usually uses an-attribute instead of anAttribute. Feel free to change it if you want to ;)

@blizzz
Copy link
Member Author

blizzz commented Sep 27, 2017

Very small nitpick: XML and HTML usually uses an-attribute instead of anAttribute. Feel free to change it if you want to ;)

Uff… did not pay attention to this. Perhaps, also because minOccurs, maxOccurs or xsi:noNamespaceSchemaLocation for instance do not follow this style as well. I acknowledge conventions and consistency however → will change this.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Coverage decreased (-0.03%) to 97.516% when pulling 9137420 on add-collaboration into 87c3421 on master.

@BernhardPosselt BernhardPosselt merged commit 8dcc30d into master Sep 27, 2017
@BernhardPosselt BernhardPosselt deleted the add-collaboration branch September 27, 2017 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants