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.

Sign up for GitHub

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

Jump to bottom

First step in decoupling GT from framework #227

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

First step in decoupling GT from framework #227

wants to merge 5 commits into from

Conversation

cies
Copy link
Member

@cies cies commented Aug 13, 2023

Fixes: #65

@cies
Copy link
Member Author

cies commented Aug 13, 2023

I see some tests are failing, while all seemed to work running ./gradlew check from the project root locally. Will look into it soon.

@cies
Copy link
Member Author

cies commented Aug 13, 2023

Fixed some, but they still go wrong if I run all tests at once. Maybe tests cannot run two instances of Play.

cies
cies commented Dec 5, 2023
View reviewed changes
fastergt/src/play/template2/JavaExtensions.java Outdated
return bytes / 1048576L + "MB";
}
return bytes / 1073741824L + "GB";
return Utils.formatSize(bytes);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in framework so it cannot be moved out. Hence I've put it in Utils.

framework/src/play/templates/Template.java
@@ -36,7 +36,7 @@ public String render(Map<String, Object> args) {
* List of arguments use in render
* @return The template result as string
*/
protected abstract String internalRender(Map<String, Object> args);
public abstract String internalRender(Map<String, Object> args);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be accessible to the fastergt library/package/code.

replay-tests/helloworld/test/ui/hello/server-error-template.html
<!DOCTYPE html>

<html>
<!doctype html>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modernize

framework/src/play/mvc/results/Error.java

private final int status;
protected final int status;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed for making NotFound and Unauthorized subclasses of Error.

framework/src/errors/404.html
@@ -1,18 +1,16 @@
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<!doctype html>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modernize

@asolntsev asolntsev added this to the 2.4.0 milestone Dec 16, 2023
@asolntsev
Copy link
Contributor

asolntsev commented Dec 24, 2023 •
edited
Loading

@cies Finally, I started reviewing this PR.
To be honest, it's hard to me to review such a big change. For example, I didn't yet understand why it was needed to move play.template.ExecutableTemplate to package play.template2 etc.

My initial idea was just to move all groovy-related code from framework to a separate module gt (like "groovy templates", similar to "fastergt"). It seemed to me a very simple and minimal change.

Do you want me to try it out? Or you have some own plan?

@asolntsev
Copy link
Contributor

@cies Look, this is my try: #316

@xabolcs xabolcs mentioned this pull request Jan 2, 2024
Merged
@cies
Copy link
Member Author

cies commented Jan 2, 2024 •
edited
Loading

@asolntsev thanks for looking into it!

I didn't yet understand why it was needed to move play.template.ExecutableTemplate to package play.template2 etc.

It's been a while, but looking at the code I think I moved it because this class depends on groovy, and the whole idea was to make framework not depend on that. I could have left it in the same package, I guess I just wanted the play.template package to be agnostic to the templating solution, or I simple did not get the meaning of the template2 package in relation to the template package.

Not moving play.template.ExecutableTemplate, FastTags, TagContext and GroovyInlineTags is --obviously-- completely okay with me. In that case we may also be able to keep Template.internalRender(...) protected (in stead of making it public).

My initial idea was just to move all groovy-related code from framework to a separate module gt (like "groovy templates", similar to "fastergt"). It seemed to me a very simple and minimal change.

That's my idea too! I was aiming a little higher: I wanted all GT-code (actual templates) to be eventually moved out of framework of which my PR was just step 1/2.

Please see the discussion of the issue.

@asolntsev asolntsev modified the milestones: 2.4.0, 2.5.0 Mar 22, 2024
@cies cies removed this from the 2.5.0 milestone May 22, 2024
@cies cies changed the title First step in decoupling GT from framework First step in decoupling GT from framework [dont merge] May 22, 2024
@cies cies changed the title First step in decoupling GT from framework [dont merge] First step in decoupling GT from framework May 23, 2024
@cies cies marked this pull request as draft May 23, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabolcs xabolcs xabolcs left review comments

Assignees

@cies cies

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove Groovy dependencies from framework
3 participants
@cies @asolntsev @xabolcs