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

Support more than one url per rule and rulegroup #312

Closed
wants to merge 3 commits into from

Conversation

PanderOpenTaal
Copy link
Contributor

@danielnaber, could you have a look at the lines with FIXME? My Java is a bit rusty and I did not manage to assert that getUrls() returns an empty list. The previous tests were if it returned null.

For some readon, my edittor atom is improving a lot of details regarding small whitespaces. This is also shipped in this pull request. It was not made intentionally but they all look legid, so please process those too.

@danielnaber
Copy link
Member

Thanks, some remarks:

  • please configure your IDE to not clean up whitespace at line ending, it makes the change set grow with useless whitespace changes
  • don't remove getUrl() and setUrl(), but deprecate them (@Deprecated) with a comment to use the new methods instead
  • RuleAsXmlSerializer: your code creates multiple url attributes, that's not valid XML I think
  • maxOccurs="unbounded" should be maxOccurs="5" or so
  • to test for an empty list, you can use assertEquals(0, getUrls.size())
  • openoffice/Main.java: have you tested this? I don't think it will work, it looks like only the last URL will be used as the other ones are overwritten by the assignment.

@PanderOpenTaal
Copy link
Contributor Author

Thanks for your review. I got immediately more into Java again. Got to work and:

  1. Will adjust IDE for next issue I work on.
  2. Put derpecated functions back.
  3. Where can I find the XSD for the file that is generated there? Where is this file read in again?
  4. Set it to the lucky number 7.
  5. Fixed and works.
  6. Fixed.
  7. NEW: I have noticed that the import is overwriting urls, foud the source and am fixing that too.

@danielnaber
Copy link
Member

The API output is according to this DTD: https://github.com/languagetool-org/languagetool/blob/master/languagetool-core/src/main/resources/org/languagetool/resource/api-output.dtd, it's used by everybody who uses the HTTP API.

@PanderOpenTaal
Copy link
Contributor Author

@danielnaber I think the last commit OpenTaal@8bc7aa6 fixes all outstandig bugs for this pull request. Could you review it again please?

@danielnaber
Copy link
Member

In the XML output, I think we have to keep the url attribute for one release, additionally to the new <url> elements. If we don't, clients will break, i.e. the URL feature would stop working on the LT homepage and in the Firefox add-on.

@danielnaber
Copy link
Member

I'm closing this, as the patch doesn't apply anymore (and has unrelated changes). Feel free to submit a new pull request.

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.

None yet

2 participants