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

Object-oriented style #2

Closed
md2perpe opened this issue Sep 1, 2016 · 7 comments
Closed

Object-oriented style #2

md2perpe opened this issue Sep 1, 2016 · 7 comments

Comments

@md2perpe
Copy link

md2perpe commented Sep 1, 2016

When I saw the example code, my direct reaction was that it should be in an object-oriented style:

package main

import (
    "fmt"
    "github.com/Luxurioust/excelize"
)

func main() {
    xlsx := excelize.CreateFile()

    sheet2 := xlsx.NewSheet(2, "Sheet2")
    sheet3 := xlsx.NewSheet(3, "Sheet3")

    sheet2.SetCellInt("A23", 10)
    sheet3.SetCellStr("B20", "Hello")

    err := xlsx.Save("~/Workbook.xlsx")
    if err != nil {
        fmt.Println(err)
    }
}
@ShawnMilo
Copy link

ShawnMilo commented Sep 1, 2016

+1

This would also cut down on typing excelize. so much, which is too long to be typing so frequently. If I used it, I think I'd override the name at import to something much shorter.

@stengaard
Copy link

stengaard commented Sep 1, 2016

If we're looking at the API anyway shouldn't Open and Save take the standard interfaces (io.Writer and io.Reader) instead of strings to filepaths. That would allow for easier composition (input could be net.Conn or http.Response.Body and outputs similar). This would mean that internally you hide the fact that you're expecting a zip.Reader, since that is the canonical representation of your data source.

Just my two cents. Fun project - thanks for sharing. :)

@ShawnMilo
Copy link

@stengaard I think that's a separate item that should be its own issue. I'd be against having that as the default -- filename strings will be used most frequently by most people, and having to "manually" deal with filehandles would be an extra burden. My vote would be for additional Write() & Read() methods which could be optionally used in the way you suggest.

@stengaard
Copy link

stengaard commented Sep 1, 2016

I put it here mostly because it would already be a breaking change to the exported API, but I agree it's not really what Per is trying to get at.

Regarding adding Read/Write: I would definitely go the other with - simplify API surface area. Specifically:

  • Introduce the Xlsx type propsed by Per and possibly (if needed) a Sheet type
  • Rename CreateFile to New - this is the common idiom. It would return an Xlsx
  • Rename OpenFile to FromReader and let it take only an io.Reader returning an Xlsx. This constructor method would be in charge of also unwrapping the zip stuff hiding that from the user.
  • Remove ReadZip and ReadZipReader. They are redundant.
  • Change and rename Save to be a method on Xlsx so that it would implement the io.WriterTo interface. This increases composability.

What I like about this project is that it is a small library. That makes it easy for the programmer to understand. I get that you're worried about leaking file-handles, but ask yourself what the core purpose of this library is? As I see it this library should allow the programmer decode, encode and manipulate XLSX data, it shouldn't be managing files since that seems peripheral to it's purpose.

But hey: your project, your rules. :)

@md2perpe
Copy link
Author

md2perpe commented Sep 3, 2016

Yes, this issue was actually only about making the API have an object oriented style.

That said, there might be other things that should also be changed. And while I think that it's good to add WriteTo(), I think that Save() should be kept. It makes the supposedly frequent operation of storing the document to disk simple.

@ericuni
Copy link

ericuni commented Aug 27, 2022

We could also consider to use number index rather than something like "A2"?

@xuri
Copy link
Member

xuri commented Aug 27, 2022

Hi @ericuni, this library provides a function named CoordinatesToCellName used for converting [X, Y] coordinates to alpha-numeric cell names.

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

No branches or pull requests

5 participants