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

Feature #529 - Allow Markup Variables In Test & Page Responder Urls #531

Merged
merged 5 commits into from Oct 26, 2014

Conversation

@mwarhaftig
Copy link
Contributor

@mwarhaftig mwarhaftig commented Oct 6, 2014

Pull request for my interpretation of how to implement this functionality.

@mwarhaftig mwarhaftig changed the title Issue #529 - Allow Markup Variables In Test & Page Responder Urls Feature #529 - Allow Markup Variables In Test & Page Responder Urls Oct 6, 2014
}

public SystemVariableSource() {
this(null);
}

public void setUrlParams(Map<String,Object> testUrlParams){

This comment has been minimized.

@amolenaar

amolenaar Oct 6, 2014
Collaborator

I like the patch (esp the acceptance criteria), only the parameters should not be added to the VariableSource, since this is shared among requests. Instead a new VariableSource instance should be created that checks for parameters and defaults to the system variable source. The SystemVariableSource should remain immutable after creation.

@mwarhaftig
Copy link
Contributor Author

@mwarhaftig mwarhaftig commented Oct 7, 2014

Ok, will update to keep SystemVariableSource immutable. ETA 10/8 (aka 8-10 localized for you :-) ).

@mwarhaftig
Copy link
Contributor Author

@mwarhaftig mwarhaftig commented Oct 9, 2014

Not using SystemVariableSource for URL params has made the WikiPageResponder implementation more complicated because the params now need to be referenced during WikiPage creation in WikiPageUtil and multiple other places. Want to better understand changes before submitting a new pull request - new ETA 10/12 (12-10).

@mwarhaftig
Copy link
Contributor Author

@mwarhaftig mwarhaftig commented Oct 12, 2014

Updated pull request to keep SystemVariableSource immutable and uses BaseWikiPage to pass URL params to ParsingPage.

@@ -89,7 +95,7 @@ public Symbol getSyntaxTree() {
private void parse() {
if (syntaxTree == null) {
// This is the only page where we need a VariableSource
parsingPage = new ParsingPage(new WikiSourcePage(this), getVariableSource());
parsingPage = new ParsingPage(new WikiSourcePage(this), getVariableSource(), urlParams);

This comment has been minimized.

@amolenaar

amolenaar Oct 13, 2014
Collaborator

I think the ParsingPage should not know about the extra map of parameters. This should all be put in the UrlPathVariableSource.

@amolenaar
Copy link
Collaborator

@amolenaar amolenaar commented Oct 13, 2014

Nice work. From the code I see that rendering a normal wiki page with those extra parameters is pretty complicated :(. I expected (/hoped) it would be easier. I added one remark about the ParsingPage, to avoid the request parameters to leak into the wikitext package. Then I think it's okay for merge.

I have some second thoughts on the setUrlParams() method, but for now it's a good starting point (due to the way VariableSource is weaved into the BaseWikiPage). I think what we need is a proper way to provide a VariableSource at the point of rendering.

@mwarhaftig
Copy link
Contributor Author

@mwarhaftig mwarhaftig commented Oct 13, 2014

I like that suggestion - keeps ParsingPage much cleaner. Updated pull request.

@amolenaar amolenaar merged commit 9999583 into unclebob:master Oct 26, 2014
amolenaar added a commit that referenced this pull request Oct 26, 2014
Feature #529 - Allow Markup Variables In Test & Page Responder Urls
@amolenaar
Copy link
Collaborator

@amolenaar amolenaar commented Oct 26, 2014

I removed the change from ParsingPage. UrlPathVariableSource already does a parameter lookup, so that should suffice. We'll have to see what to do about BaseWikiPage.setUrlParams(). I don't like the fact we "set" stuff on the page, but the way VariableSource is handled by the wiki pages leaves us no alternative.

@roboaks
Copy link

@roboaks roboaks commented Oct 27, 2014

Thanks so much for your work on this feature. It will be a great benefit to our team and, I suspect, many others.

@amolenaar amolenaar added this to the Next release milestone Nov 13, 2014
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

3 participants