这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@rubinstory
Copy link

Changed: Refactor LocalClientSocket

rubinstory and others added 3 commits May 30, 2022 01:28
Since `getLogString()` and `getMarkdownString()` has same log-appeding
structure, added `getLogVariableList()` which returns a list of
`Pair<String, Object>` that contains variables for creating logs.
`getMarkdownString()` in `ReportInfo.java`, `LocalClientSocket.java` and
`LocalSocketRunConfig.java`.

Add variables `label` and `object` in `getLogString()` and
`getMarkdownString()` to explain which variables are used.
@agnostic-apollo
Copy link
Member

agnostic-apollo commented May 30, 2022

This wasn't properly rebased and you are pushing merge commits to all your pull requests.

Secondly, with this getLogVariableList() you are doing everywhere is requiring case statements to handle special cases and would fail for complex cases like ExecutionCommand.

Instead, create specialized classes LogStringBuilder and MarkdownStringBuilder under com.termux.shared.data package that stores a StringBuilder object. This class provides append methods for Logger.getSingleLineLogStringEntry() and MarkdownUtils.getSingleLineMarkdownStringEntry() and multiline variants. So something like

StringBuilder logString = new StringBuilder();
logString.append("\n").append(Logger.getSingleLineLogStringEntry("Path", mPath, "-"));

would be like

LogStringBuilder logString = new LogStringBuilder();
logString.appendSLEntry("Path", mPath);

The function will automatically add \n and use - as default, you can also add another function that allows default value to be passed. A simple append() method without any newline addition should also be added, also appendTitle() and appendSubtitle() functions.

Both the classes could actually implement an interface ObjectStringBuilder that contains all the functions, this could allow something like JsonStringBuilder and YamlStringBuilder to be added in future as well. Some simple implementations for objects of log and markdown strings could also be simplified into a single function by passing ObjectStringBuilder as parameter to the function like following...

getLogString() {
    getObjectString(new LogStringBuilder());
}

getMarkdownString() {
    getObjectString(new MarkdownStringBuilder());
}

getObjectString(ObjectStringBuilder builder) {
    builder.appendSLEntry("Path", mPath);
}

You can create a single pull request for the log and markdown string replacements.

@rubinstory
Copy link
Author

@agnostic-apollo
Thanks for reviewing. We'll fix our commits with correctly rebasing and applying those Builder pattern you've said.

@agnostic-apollo
Copy link
Member

You are welcome. Let me know if you need any help or something is unclear.

@rubinstory rubinstory closed this May 30, 2022
@rubinstory
Copy link
Author

@agnostic-apollo
Currently, I've almost finished adding ObjectStringBuilder and its child classes. But I can't understand the usage of appendSubtitle() function. Could you give me some examples?

@agnostic-apollo
Copy link
Member

Sometimes there are sub sections that have

with nested keys

logString.append(mTitle).append(" Socket Server Run Config:");

logString.append(localSocketRunConfig.getTitle()).append(" Socket Server Error:\n");

or content

markdownString.append("\n\n### Command Description\n\n").append(executionCommand.commandDescription).append("\n");

For markdown, heading size depends on # used and I close sections with \n##\n\n for horizontal bar.
For log strings, : is being used, and closing with \n\n\n.

For json and yaml, this could be implemented as keys instead, but that would require proper opening and closing.
For markdown, nesting could increasing number of #, like execution command does.

I haven't thought this too much, there is probably a way for all this. You can skip it for now. We can hard code it for markdown's case for now, since execution command doesn't log that stuff for log string and only markdown.

@agnostic-apollo
Copy link
Member

Hey @rubinstory, was there any progress on this? ExecutionCommand is becoming more and more of a mess with things being added and I am thinking on adding ObjectStringBuilder now. Even if its not complete, just send a patch file and I'll look into merging it locally. Otherwise, will just implement it myself, no worries. Thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants