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

Implement 1356 #1572

Merged
merged 10 commits into from
Jul 13, 2016
Merged

Implement 1356 #1572

merged 10 commits into from
Jul 13, 2016

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Jul 11, 2016

Implements #1356 and provides a formatter that converts HTML to Unicode (converting characters and eliminating tags)

  • Change in CHANGELOG.md described
  • Tests created for changes

@simonharrer
Copy link
Contributor

LGTM

@@ -2279,6 +2279,7 @@ Changes_the_first_letter_of_all_words_to_capital_case_and_the_remaining_letters_
Cleans_up_LaTeX_code.=Räumt_LaTeX-Code_auf.

Converts_HTML_code_to_LaTeX_code.=Konvertiere_HTML-Code_in_LaTeX-Code.
Converts_HTML_code_to_Unicode.= Konvertiere_HTML-Code_in_Unicode.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a space after the = sign?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed! Thanks for spotting!

@stefan-kolb
Copy link
Member

LGTM 👍


@Before
public void setUp() throws Exception {
Globals.prefs = JabRefPreferences.getInstance();
Copy link
Member

Choose a reason for hiding this comment

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

It is best practice to init the class under test in the @Before method so that every test method gets a fresh class, i.e move the = new HtmlToUnicodeFormatter part to this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct. That comes from a copy paste from the other formatter tests, which all share this problem. I will fix all of them in this PR.

@tobiasdiez
Copy link
Member

tobiasdiez commented Jul 11, 2016

I have only a few small remarks. 👍 for merge in general

@lenhard lenhard merged commit 190497d into master Jul 13, 2016
@stefan-kolb stefan-kolb deleted the implement-1356 branch July 13, 2016 21:43
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.

5 participants