You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "michael-o (via GitHub)" <gi...@apache.org> on 2023/08/15 13:17:17 UTC
[GitHub] [commons-text] michael-o opened a new pull request, #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
michael-o opened a new pull request, #449:
URL: https://github.com/apache/commons-text/pull/449
This closes #449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] garydgregory commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1689936332
> @elharo, I have copied a pre-existing class which uses the same pattern as other classes. I won't change a single class, this needs to be done for all of them in a separate PR.
Some constructors are private, some are not, you just happen to have picked a class for your template that had its constructor package-private instead to private.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] garydgregory commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1685309351
Since this PR relies on `StringEscapeUtils.escapeXml10(key)` and XML 1.1 exists, shouldn't we either support both or change the names in this PR to explicitly call out XML 1.0? For example, `DefaultStringLookup.XML_DECODER` -> `DefaultStringLookup.XML_DECODER_1_0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] garydgregory merged pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #449:
URL: https://github.com/apache/commons-text/pull/449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] elharo commented on a diff in pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #449:
URL: https://github.com/apache/commons-text/pull/449#discussion_r1299407880
##########
src/test/java/org/apache/commons/text/lookup/XmlDecoderStringLookupTest.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+
+package org.apache.commons.text.lookup;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link XmlDecoderStringLookup}.
+ */
+public class XmlDecoderStringLookupTest {
+
+ private static final String DATA = "<element>";
Review Comment:
inline. DATA is much less clear than the literal string. Named constants are for numbers, not strings.
##########
src/test/java/org/apache/commons/text/lookup/XmlEncoderStringLookupTest.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+
+package org.apache.commons.text.lookup;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link XmlEncoderStringLookup}.
+ */
+public class XmlEncoderStringLookupTest {
+
+ private static final String DATA = "<element>";
Review Comment:
inline. DATA is much less clear than the literal string. Named constants are for numbers, not strings.
##########
src/main/java/org/apache/commons/text/lookup/XmlEncoderStringLookup.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+
+package org.apache.commons.text.lookup;
+
+import org.apache.commons.text.StringEscapeUtils;
+
+/**
+ * Encodes strings according to the XML 1.0 specification.
+ *
+ * @see StringEscapeUtils#escapeXml10(String)
+ * @since 1.11.0
+ */
+final class XmlEncoderStringLookup extends AbstractStringLookup {
+
+ /**
+ * Defines the singleton for this class.
+ */
+ static final XmlEncoderStringLookup INSTANCE = new XmlEncoderStringLookup();
+
+ XmlEncoderStringLookup() {
Review Comment:
private
##########
src/main/java/org/apache/commons/text/lookup/XmlDecoderStringLookup.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+
+package org.apache.commons.text.lookup;
+
+import org.apache.commons.text.StringEscapeUtils;
+
+/**
+ * Decodes strings according to the XML 1.0 specification.
+ *
+ * @see StringEscapeUtils#unescapeXml(String)
+ * @since 1.11.0
+ */
+final class XmlDecoderStringLookup extends AbstractStringLookup {
+
+ /**
+ * Defines the singleton for this class.
+ */
+ static final XmlDecoderStringLookup INSTANCE = new XmlDecoderStringLookup();
+
+ XmlDecoderStringLookup() {
Review Comment:
private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] garydgregory commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1689981142
https://issues.apache.org/jira/projects/TEXT/issues/TEXT-229
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] michael-o commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1685361479
@elharo, I have copied a pre-existing class which uses the same pattern as other classes. I won't change a single class, this needs to be done for all of them in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] elharo commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1685408189
I cry [Emerson](https://www.goodreads.com/quotes/353571-a-foolish-consistency-is-the-hobgoblin-of-little-minds-adored). Consistency with bad code is not a virtue. I see no reason to make the new API excessively complex, public, and hard-to-read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] michael-o commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1685322398
I'd say that 1.0 should be the default since 1.1 gained little traction. If someone wants later 1.1 support we can add, but given that no one even requested 1.0 for the past years I am reluctant to rename those coders.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-text] codecov-commenter commented on pull request #449: [TEXT-229] Add XmlEncoderStringLookup/XmlDecoderStringLookup
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #449:
URL: https://github.com/apache/commons-text/pull/449#issuecomment-1689935391
## [Codecov](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#449](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b5d2aed) into [master](https://app.codecov.io/gh/apache/commons-text/commit/28b6e1d38a47cab1b6beaadd8585780b49a178ef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (28b6e1d) will **increase** coverage by `0.15%`.
> Report is 5 commits behind head on master.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #449 +/- ##
============================================
+ Coverage 97.13% 97.29% +0.15%
- Complexity 2332 2655 +323
============================================
Files 84 86 +2
Lines 5769 6333 +564
Branches 936 1077 +141
============================================
+ Hits 5604 6162 +558
- Misses 87 91 +4
- Partials 78 80 +2
```
| [Files Changed](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/commons/text/StringSubstitutor.java](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvdGV4dC9TdHJpbmdTdWJzdGl0dXRvci5qYXZh) | `95.20% <ø> (ø)` | |
| [...pache/commons/text/lookup/DefaultStringLookup.java](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvdGV4dC9sb29rdXAvRGVmYXVsdFN0cmluZ0xvb2t1cC5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...pache/commons/text/lookup/StringLookupFactory.java](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvdGV4dC9sb29rdXAvU3RyaW5nTG9va3VwRmFjdG9yeS5qYXZh) | `96.42% <100.00%> (+0.17%)` | :arrow_up: |
| [...he/commons/text/lookup/XmlDecoderStringLookup.java](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvdGV4dC9sb29rdXAvWG1sRGVjb2RlclN0cmluZ0xvb2t1cC5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...he/commons/text/lookup/XmlEncoderStringLookup.java](https://app.codecov.io/gh/apache/commons-text/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvdGV4dC9sb29rdXAvWG1sRW5jb2RlclN0cmluZ0xvb2t1cC5qYXZh) | `100.00% <100.00%> (ø)` | |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/commons-text/pull/449/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org