[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user asfgit closed the pull request at: https://github.com/apache/metamodel/pull/122 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/122#discussion_r74330944
--- Diff:
fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/EbcdicReader.java ---
@@ -26,13 +26,17 @@
*/
class EbcdicReader extends FixedWidthReader {
+private final BufferedInputStream _stream;
+private final String _charsetName;
private final boolean _skipEbcdicHeader;
private final boolean _eolPresent;
private boolean _headerSkipped;
public EbcdicReader(BufferedInputStream stream, String charsetName,
int[] valueWidths,
boolean failOnInconsistentLineWidth, boolean skipEbcdicHeader,
boolean eolPresent) {
super(stream, charsetName, valueWidths,
failOnInconsistentLineWidth);
+_stream = stream;
+_charsetName = charsetName;
--- End diff --
Ah yes, it was fooling me because it used to be the super-class' fields and
now they've moved here... Makes sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/122#discussion_r74198110
--- Diff:
fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/EbcdicReader.java ---
@@ -26,13 +26,17 @@
*/
class EbcdicReader extends FixedWidthReader {
+private final BufferedInputStream _stream;
+private final String _charsetName;
private final boolean _skipEbcdicHeader;
private final boolean _eolPresent;
private boolean _headerSkipped;
public EbcdicReader(BufferedInputStream stream, String charsetName,
int[] valueWidths,
boolean failOnInconsistentLineWidth, boolean skipEbcdicHeader,
boolean eolPresent) {
super(stream, charsetName, valueWidths,
failOnInconsistentLineWidth);
+_stream = stream;
+_charsetName = charsetName;
--- End diff --
But they are, lines: 61, 70, 76.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/122#discussion_r74183535
--- Diff:
fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/EbcdicReader.java ---
@@ -26,13 +26,17 @@
*/
class EbcdicReader extends FixedWidthReader {
+private final BufferedInputStream _stream;
+private final String _charsetName;
private final boolean _skipEbcdicHeader;
private final boolean _eolPresent;
private boolean _headerSkipped;
public EbcdicReader(BufferedInputStream stream, String charsetName,
int[] valueWidths,
boolean failOnInconsistentLineWidth, boolean skipEbcdicHeader,
boolean eolPresent) {
super(stream, charsetName, valueWidths,
failOnInconsistentLineWidth);
+_stream = stream;
+_charsetName = charsetName;
--- End diff --
These two don't seem to ever be used?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user jhorcicka commented on a diff in the pull request: https://github.com/apache/metamodel/pull/122#discussion_r73830409 --- Diff: fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/FixedWidthReader.java --- @@ -43,6 +47,7 @@ private final boolean _constantWidth; private volatile int _rowNumber; protected final BufferedInputStream _stream; +protected Reader _reader; --- End diff -- Done. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user kaspersorensen commented on a diff in the pull request: https://github.com/apache/metamodel/pull/122#discussion_r73817859 --- Diff: fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/FixedWidthReader.java --- @@ -43,6 +47,7 @@ private final boolean _constantWidth; private volatile int _rowNumber; protected final BufferedInputStream _stream; +protected Reader _reader; --- End diff -- I would suggest to keep having final fields only, and since `_stream` and `_charset` are no longer really used, I'd suggest to remove those and instead make the `initReader` return the created Reader with the needed arguments, like this: ``` _reader = initReader(stream, charsetName); ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/122#discussion_r73656991
--- Diff:
fixedwidth/src/main/java/org/apache/metamodel/fixedwidth/FixedWidthReader.java
---
@@ -85,6 +92,15 @@ public FixedWidthReader(InputStream stream, String
charsetName, int[] valueWidth
_expectedLineLength = expectedLineLength;
}
+private void initReader() {
+try {
+InputStreamReader inputStreamReader = new
InputStreamReader(_stream, _charsetName);
--- End diff --
This was the missing part in the previous version, _charsetName was not
used anywhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #122: METAMODEL-1109: FixedWidthReader diacritics bug...
GitHub user jhorcicka opened a pull request: https://github.com/apache/metamodel/pull/122 METAMODEL-1109: FixedWidthReader diacritics bug fix Refactoring of FixedWidthReader in PR #103 introduced a bug related to charset (which was not used). So, input containing diacritic characters was not read correctly. JUnit FixedWidthReaderTest was updated with charset tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jhorcicka/metamodel METAMODEL-1109-FixedWidthReader-diacritics-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/metamodel/pull/122.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #122 commit e23f4877b93bd39e4daf7dd17740c252f6f39ab2 Author: jhorcicka Date: 2016-08-05T07:51:18Z METAMODEL-1109: FixedWidthReader diacritics bug fix (_charsetName was not used). Stream was replaced by a reader. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
