Skip to content

Conversation

@LionNatsu
Copy link
Contributor

@LionNatsu LionNatsu commented Feb 2, 2017

Changes proposed in this pull request:

  • replacing and cleaning the fields and constants which are in unexported structs, or are only used for internal namespace.
    • 9bf62b9 opcode.go: unexport opcode constants
    • 0d3d1e4 type CompileError: unexport context - funcContext is unexported
    • 928e92c compile.go: unexport fields in codeBlock, funcContext
    • b8b16ae compile.go: unexport fields in varNamePoolValue
    • 7438224 compile.go: add comment for Value in constValueExpr
    • 044e437 opcode.go: unexport fields in opProp
    • 71264c1 pm.go: unexport fields in scannerState, scanner
    • 735b8af pm.go: unexport fields in inst
    • 8a8fe4c pm.go: unexport fields in unexported structs
    • 124ef63 state.go: unexport fields in callFrame
    • 5ddcf15 utils.go: unexport fields in flagScanner
    • e0f35fc stringlib.go: unexport fields in replaceInfo
  • minor changes for readability of code
    • 47b34d0 channellib.go: use named field to compose literal
    • 35aa627 readBufio*: reorder return; remove redundant conditions
    • ed5dbe5 utils.go: refactor logic flow of flagScanner.Next & parseNumber

@coveralls
Copy link

coveralls commented Feb 2, 2017

Coverage Status

Coverage increased (+0.001%) to 90.476% when pulling 124ef63 on LionNatsu:patch-lint into 66c871e on yuin:master.

@coveralls
Copy link

coveralls commented Feb 2, 2017

Coverage Status

Coverage increased (+0.001%) to 90.476% when pulling e0f35fc on LionNatsu:patch-lint into 66c871e on yuin:master.

@yuin
Copy link
Owner

yuin commented Feb 2, 2017

"Readability" is somewhat subjective.
And if apply this kind of large refactoring, I'll do it by myself. If I don't do that, I'll probably be confused about GopherLua code base.

@yuin yuin closed this Feb 2, 2017
@LionNatsu
Copy link
Contributor Author

Thanks for your reviewing. The PR has been rejected but I still want to explain the refactoring a little bit more here.

The 'unexport' refactoring is that, the capitalized fields could be only used for exported struct. Actually, most of the fields in GopherLua should be unexported because they're internal struct. This massive refactoring mainly lowercase them. And all unexported struct except for two, constValueExpr and lValue…(sorry I forgot the name).

It's true that something can be confusing: Class conflicts with class so I turned it cls, String could be confusing with predefined type string so use str… There're two ore three changes in this not consistent way.

I'm sorry for confusing you. Anyway I hope these trivial minor changes could be useful for your consideration.

Happy new year! (From my hometown)

@yuin
Copy link
Owner

yuin commented Feb 3, 2017

Thanks for your detailed explanations.

The reason that I have used capitalized fileds for unexported structs, these fields are exported for structs.
And I have used lower case fileds if these fileds are unexported for structs.

For example:

we have an unexported struct like the following:

type user struct {
  name string
  age    int
  somePrivateValue int
}

This struct is unexported for now .

Consider the case that we want to export this struct for some reason in the future.

type User struct {
  name string
  age    int
  somePrivateValue int
}

It's pointless. name and age should also be exported.

type User struct {
  Name string
  Age    int
  somePrivateValue int
}

So, I am using capitalized fields for unexported structs while taking that into account. This is a bad idea?

@LionNatsu
Copy link
Contributor Author

LionNatsu commented Feb 3, 2017

No, it isn't. It is good.

But it's time to come to some details that the PR has three kind of change here about 'unexport':

  1. to unexport constants can never be used (OP_CODEs)
  2. to unexport fields in unexported struct
  3. to unexport fields as neither exported struct nor exported interface

Let's take a look at 2&3.

To unexport fields in unexported struct

type user struct {
	Name             string
	Age              int
	somePrivateValue int
}

This is the most thing in the PR. Yes, your idea is definitely okay. And further more, to change or not, actually, will never affect the 'surface' of package. I think I got your point of keeping this. I will revert them in my repo.

To unexport fields as neither exported struct nor exported interface

Excactly, there is only one in this kind, but also is the worst one.
commit: 0d3d1e4 type CompileError: unexport context - funcContext is unexported

type CompileError struct { // {{{
	Context *funcContext
	Line    int
	Message string
}

Well, it could be very confusing programmers in at least these two reason:

  • "I have no idea about what funcContext is, maybe we have to read the source to find this internal struct."
  • "We may use this without the knowledge of funcContext, but it seems like a black box."

Obviously, there are two ways to improve it:

  1. unexport Context to context
  2. export funcContext to FuncContext

I prefer the first one. the second one could lead to more massive refactoring, and 'pull back' the line between external and internal. The most important thing is that a struct for external error report should not export such internal mechanism.

Got any thoughts on this one? I'm all ears.

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.

3 participants