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

Conversation

Projects
None yet
3 participants
@blizzz
Member

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.

add collaboration type
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

@blizzz blizzz added the enhancement label Sep 18, 2017

@blizzz blizzz requested a review from BernhardPosselt Sep 18, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 18, 2017

Coverage Status

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

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

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Sep 19, 2017

Member

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.

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.

simplify XML structure and introduce enums for valid attribute values
also add some doc

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

@blizzz blizzz referenced this pull request Sep 26, 2017

Merged

adapt info.xml #135

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Sep 26, 2017

Member

@BernhardPosselt ty! Your remarks are addressed

Member

blizzz commented Sep 26, 2017

@BernhardPosselt ty! Your remarks are addressed

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 26, 2017

Coverage Status

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

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

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Sep 27, 2017

Member

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

Member

BernhardPosselt 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 ;)

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Sep 27, 2017

Member

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.

Member

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.

change attribute name from camel to kebab case
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 27, 2017

Coverage Status

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 97.516%
Details

@BernhardPosselt BernhardPosselt deleted the add-collaboration branch Sep 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment