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

Extend MapToolScriptTokenMaker #241

Merged
merged 5 commits into from Feb 14, 2019

Conversation

JamzTheMan
Copy link
Member

@JamzTheMan JamzTheMan commented Feb 12, 2019

PR for Issue #239 & #240


This change is Reviewable

Signed-off-by: Jamz <Jamz@Nerps.net>
 * MapToolScriptSyntax now dynamically adds in all function names
retrieved by the MapToolLineParser
 * AutoCompletion turned on for Macro Editor! meta+space to activate
   * Macro short descriptions and summary text needs to be added.
Currently loading from a properties file.
 * Some refactoring...

Signed-off-by: Jamz <Jamz@Nerps.net>
Signed-off-by: Jamz <Jamz@Nerps.net>
@JamzTheMan JamzTheMan added the feature Adding functionality that adds value label Feb 12, 2019
@JamzTheMan JamzTheMan self-assigned this Feb 12, 2019
@JamzTheMan JamzTheMan added this to In progress in Merging Nerps -> RPtools via automation Feb 12, 2019
Signed-off-by: Jamz <Jamz@Nerps.net>
Copy link
Member

@Azhrei Azhrei left a comment

Choose a reason for hiding this comment

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

My only concerns are regarding the i18n stuff. I don't see any value in putting getString(String, ResourceBundle) into I18N. And the macro properties file has a really large string for "abort.summary"; please break it up by putting backslash-newline after each "\n" in the string. This will allow the text to span multiple physical lines. Thanks!

Reviewed 17 of 19 files at r1.
Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @Azhrei and @cwisniew)

Copy link
Member Author

@JamzTheMan JamzTheMan left a comment

Choose a reason for hiding this comment

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

To be honest, was kinda of throwing it together. adding the new getString just seemed like a way to reuse the i18n stuff in case we wanted to break things up more in the future. Do you just want me to add the static final location in the i18n instead? I have no strong feeling either way.

What's the purpose of breaking up the .properties line? No strong feelings but curious why? I honestly just did a quick/dirty copy from HTML source there as well just for a sample and test how it will look. It looked fine but it will obviously change once we get a real dump of the source.

Reviewable status: 17 of 19 files reviewed, all discussions resolved (waiting on @cwisniew)

@JamzTheMan
Copy link
Member Author

I'm going to merge this for now but consider it WIP and may change as we solidify how to handle macro descriptions.

@JamzTheMan
Copy link
Member Author

Ok not sure why Travis reports yellow, it completed successfully. I will return job and see what happens.

@Azhrei
Copy link
Member

Azhrei commented Feb 13, 2019

I18N might be fine place for it, but then some reorg is needed.

The long line will freak out some tools (like GH Reviewable!) and makes it difficult to see changes to one word (because the whole line is in the diff).

No big deal from a functionality standpoint.

@JamzTheMan
Copy link
Member Author

What reorg do you have in mind?

And all valid points re long line, makes sense, I can make that happen.

Signed-off-by: Jamz <Jamz@Nerps.net>
@JamzTheMan JamzTheMan merged commit d0f2ccf into RPTools:merge-from-nerps Feb 14, 2019
Merging Nerps -> RPtools automation moved this from In progress to Done Feb 14, 2019
@JamzTheMan JamzTheMan moved this from Done to Merged in Merging Nerps -> RPtools Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding functionality that adds value
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants