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

Refactored to make adding keywords easier, fixes #333 #335

Closed
wants to merge 1 commit into from

Conversation

fuhrmanator
Copy link
Contributor

Hope you don't mind the refactor - it should make it easier to maintain these.

@qjebbs
Copy link
Owner

qjebbs commented Aug 12, 2020

You know what, I keep the keywords in a string, because it's compatible (and same process) to dealing with output of java -jar plantuml.jar -language, in this way, we prompt keywords only current jar supports.

For some reason, I temporary remove this logic, but I may improve and bring it back some day.

function getLanguageWords(): Promise<LanguageWord[]> {
// clear dict
dicLanguageWords = new Set<string>([]);
let task: Promise<string>;
if (config.render !== RenderType.Local || !config.java) {
task = Promise.resolve(preDefinedWords);
} else {
let params = [
'-Djava.awt.headless=true',
'-jar',
// FIXME: this would cause false alarm of invalid jar, if user has jar setting in workspace.
config.jar(null),
"-language",
];
let process = child_process.spawn(config.java, params);
task = processWrapper(process).then(
buf => buf.toString(),
e => preDefinedWords
)
}
return task.then(
words => processWords(words)
)
}

So to fix the issue, the simplest way is replacing preDefinedWords with latest output of java -jar plantuml.jar -language

@fuhrmanator
Copy link
Contributor Author

There is still a problem then with #333 - many of those words don't show up in the latest list (strictuml, italic, bold, circle, description, ...). Perhaps there's a way to merge the two methods?

@fuhrmanator
Copy link
Contributor Author

Or maybe we can pressure PlantUML maintainer to update that list (upstream bug).

@qjebbs
Copy link
Owner

qjebbs commented Aug 12, 2020

May I suggest you add missing keyword manually, and comment with "Don't replace this with plantuml.jar output" or something? Until the upstream bug is resolved.

@fuhrmanator
Copy link
Contributor Author

May I suggest you add missing keyword manually, and comment with "Don't replace this with plantuml.jar output" or something? Until the upstream bug is resolved.

It's been resolved for the short term (but could occur again, because when a new keyword is added, it must be also added to a list of keywords). See the discussion at plantuml/plantuml#359

I think it's better to depend on the content of https://github.com/plantuml/plantuml/blob/master/src/net/sourceforge/plantuml/syntax/LanguageDescriptor.java rather than reproducing the keywords here.

@fuhrmanator fuhrmanator closed this Oct 9, 2020
@qjebbs
Copy link
Owner

qjebbs commented Oct 10, 2020

The point is, I cannot trace the language changes all the time. PlantUML project have more people and keywords comes from them. If the upstream can miss some keywords, how can anyone expect me alone to do better than them, to not miss something.

Now that upstream provides a keywords command, my best choice is to depend on it.

Since we are digging this so deep, I could say, we can use both custom keywords array and generating. We merge two sets of keywords with program logic, so that we can do something on both ends when this issue occurs.

qjebbs added a commit that referenced this pull request Oct 10, 2020
qjebbs added a commit that referenced this pull request Nov 12, 2020
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