Skip to content
This repository was archived by the owner on Mar 6, 2019. It is now read-only.

Read filing's contents using specified encoding#86

Open
myersjustinc wants to merge 1 commit intonytimes:masterfrom
associatedpress:read-filing-with-encoding
Open

Read filing's contents using specified encoding#86
myersjustinc wants to merge 1 commit intonytimes:masterfrom
associatedpress:read-filing-with-encoding

Conversation

@myersjustinc
Copy link

The @encoding instance variable on a Filing object is ignored in methods such as
Filing#form_type, which can lead to an ArgumentError ("invalid byte sequence in UTF-8"). Before the included change in lib/fech/filing.rb is made, the included test case demonstrates such an error when we try to call Filing#summary.

This change takes @encoding into account when reading the filing from disk, which avoids the ArgumentError.

(The entire test suite now only has two failing tests, both of which are addressed in #83 and don't appear to be related to this specific change.)

The `@encoding` instance variable on a `Filing` object is ignored in
methods such as `Filing#form_type`, which can lead to an `ArgumentError`
("invalid byte sequence in UTF-8"). The included test case demonstrates
such an error when we try to call `Filing#summary`.

This change takes `@encoding` into account when reading the filing from
disk, which avoids the `ArgumentError`.
@dwillis
Copy link
Contributor

dwillis commented Feb 7, 2019

@myersjustinc Thanks for this! I am not sure if the folks at the NYT are still maintaining this, but I've got a fork here and would be happy to have this PR there.

@myersjustinc
Copy link
Author

Sounds good. Once I've filed a matching PR on dwillis/Fech, should I also leave this one open on this repo, or should I close this one to avoid confusion?

@dwillis
Copy link
Contributor

dwillis commented Feb 7, 2019

I would leave it open, as I'm unsure whether this repo is actively monitored.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants