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

occ: Provide arguments for link and linktext #172

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

guruz
Copy link
Contributor

@guruz guruz commented Apr 9, 2018

For owncloud/client#6240

Do you agree with @dragotin 's idea from the client PR to put the link text into the link property too?

notifications:generate [-u|--user USER] [-g|--group GROUP] [--] <subject> [<message>] [<link>] [<linktext>]

@DeepDiver1975
Copy link
Member

Add my comment to the client pr - I consider this magic whitespace as fishy

@jvillafanez
Copy link
Member

I think everyone agree that the setLink expects an html link to be used as parameter, so anything set there should be interpreted as such. Changing the meaning doesn't seem a good option.
There is the additional problem of lacking specifications (the interface doesn't say anything about whether the link should be urlencoded or not, or who should ensure that), so a whitespace could be a valid character as part of the link if we consider that the encoding should be done by the one rendering the link.

I'd prefer to remove the text link for now until we have proper support for it. Just setting the link should be good enough.

Regardless the code itself, I'm not sure if the link should be an argument or an option. Argument feels like it's mandatory even though it isn't.

@guruz
Copy link
Contributor Author

guruz commented Apr 12, 2018

@jvillafanez @DeepDiver1975 PR updated

@DeepDiver1975 DeepDiver1975 merged commit 80112f7 into stable10 Apr 12, 2018
@DeepDiver1975 DeepDiver1975 deleted the link_argument_occ branch April 12, 2018 14:11
@DeepDiver1975 DeepDiver1975 added this to the development milestone Apr 12, 2018
@DeepDiver1975
Copy link
Member

@guruz please backport to stable10 - THX

@guruz
Copy link
Contributor Author

guruz commented Apr 13, 2018

@DeepDiver1975 the commit was in stable10.

@guruz
Copy link
Contributor Author

guruz commented Apr 13, 2018

are you merging stable10 to master usually?

@PVince81
Copy link
Contributor

@guruz not yet for bundled apps. For marketplace apps, yes

@guruz
Copy link
Contributor Author

guruz commented Apr 18, 2018

front port..ahja

@mmattel
Copy link
Contributor

mmattel commented Apr 20, 2018

@PVince81 PVince81 modified the milestones: development, QA Jun 13, 2018
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.

5 participants