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

Added support to load from memory #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

racerxdl
Copy link

@racerxdl racerxdl commented Mar 7, 2019

I added support to load a shapefile from memory. This way if you store the shp/dbf files using go-bindata then you can load it using byte arrays.

    shpFileData, err := ImageData.Asset("ne_50m_admin_0_countries.shp")
    if err != nil {
        panic(err)
    }
    dbfFileData, err := ImageData.Asset("ne_50m_admin_0_countries.dbf")
    if err != nil {
        panic(err)
    }
    shape, err := shp.OpenFromMemory(shpFileData, dbfFileData)
    if err != nil {
        return err, nil
    }
    defer shape.Close()

@@ -53,6 +75,17 @@ func Open(filename string) (*Reader, error) {
return s, nil
}

func OpenFromMemory(shpFileData, dbfFileData []byte) (*Reader, error) {

Choose a reason for hiding this comment

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

exported function OpenFromMemory should have comment or be unexported

@codecov-io
Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #36 into master will increase coverage by 0.12%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   60.04%   60.17%   +0.12%     
==========================================
  Files           7        7              
  Lines         801      816      +15     
==========================================
+ Hits          481      491      +10     
- Misses        264      269       +5     
  Partials       56       56
Impacted Files Coverage Δ
reader.go 61.64% <55.55%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c721ed4...dc51776. Read the comment docs.

reader.go Show resolved Hide resolved
Copy link
Collaborator

@fawick fawick left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. I understand that you want to read shapefiles and attributes from data embedded in the executable.

To be honest, the proposed solution feels too "hacky" for me. I am sure that it works, but I'm afraid this could be hard to maintain in the long run.

I'd rather you'd open up an issue to discuss a design. I'd also be interested to get more minds on this.

@@ -30,6 +31,8 @@ type Reader struct {
dbfNumRecords int32
dbfHeaderLength int16
dbfRecordLength int16

memoryData *memoryShapeData
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not happy with adding this member variable to the struct.

Do you need to have random access? If not, have you considered https://godoc.org/github.com/jonas-p/go-shp#SequentialReaderFromExt for opening the byte slices?

r.dbf, err = os.Open(r.filename + ".dbf")
if err != nil {
return
if r.memoryData != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO this is a bit of a code smell: Testing whether one of the members is holding data and then branching the logic from there.

dbfFileData: dbfFileData,
}

s := &Reader{filename: "", shp: newBytesReader(mD.shpFileData), memoryData: mD}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have doubts about this, as well. Effectively, you are adding references to mD twice to s. I understand that you want to preserve the dbf data so that openDbf has something to open later, but the proposed design will overcomplicate things.

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.

4 participants