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

all: moves to camel case naming #38

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

all: moves to camel case naming #38

wants to merge 2 commits into from

Conversation

donkahlero
Copy link
Collaborator

Hi guys,
I had a stab at #37 and renamed the variables. Took me a bit longer than expected as so many variables are affected and maybe I did not even catch them all...

Nevertheless, some points are still up for discussion and I hope we can discuss them as part of this PR:

What shall we do about the Datatype vs. DataType issue? I saw that @sbinet had once a stab at this. I still suggest that we move what is called Datatype to DataType as part of this PR and then rethink later if we want to move to what you said in #9, @kortschak. The same goes for Dataspace which should maybe also be called DataSpace?

Id vs. ID. Right now I moved everything to Id, but I am not quite sure if ID is the better way. The go standard lib uses the prior while golint suggest the latter. Any opinion?

_go_types in h5t_types.go. Should we rename e.g. _go_int8_t to goInt8T?

ALL_CAPS_VARS as we have them in h5t_shim.go should stay as they are, right?

This patch is in regard to issue #37
and features a major renaming of all affected variables.
@kortschak
Copy link
Member

kortschak commented Jul 8, 2018

This will answer some of your questions.

Data Type is two words, so it can be made DataType, though do think Type is better.

Dataspace is a single compound word, so it should remain (English is weird).

ID should be ID - as described in the wiki.

For the reflect.Type values, s/_go_(.*)_t/\1Type/g.

It seems reasonable to keep the T_ vars and consts as they are.

@@ -67,20 +67,20 @@ func main() {
panic(err)
}
defer f.Close()
fmt.Printf(":: file [%s] created (id=%d)\n", fname, f.ID())
fmt.Printf(":: file [%s] created (id=%d)\n", fname, f.Id())
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the ID changes.


// create the memory data type
dtype, err := hdf5.NewDatatypeFromValue(s1[0])
dType, err := hdf5.NewDatatypeFromValue(s1[0])
Copy link
Member

Choose a reason for hiding this comment

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

I'd be really happy if these all just became typ.

h5a_attribute.go Outdated
defer C.free(unsafe.Pointer(c_name))
hid := C.H5Acreate2(id, c_name, dtype.id, dspace.id, acpl.id, P_DEFAULT.id)
if err := checkID(hid); err != nil {
func createAttribute(id C.hid_t, name string, dType *Datatype, dSpace *Dataspace, acpl *PropList) (*Attribute, error) {
Copy link
Member

Choose a reason for hiding this comment

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

And, for the Dataspace labels, just spc.

h5d_dataset.go Outdated
var filespace_id, memspace_id C.hid_t = 0, 0
if memspace != nil {
memspace_id = memspace.id
var fileSpaceId, memSpaceId C.hid_t = 0, 0
Copy link
Member

Choose a reason for hiding this comment

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

Remove the redundant assignment and change to fileSpaceID, memSpaceID.

func (s *Dataset) ReadSubset(data interface{}, memspace, filespace *Dataspace) error {
dtype, err := s.Datatype()
defer dtype.Close()
func (s *Dataset) ReadSubset(data interface{}, memSpace, fileSpace *Dataspace) error {
Copy link
Member

Choose a reason for hiding this comment

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

These are good though.

h5t_types.go Outdated
if err != nil {
return nil, err
}
var ptrPathLen int
n := t.NumField()
for i := 0; i < n; i++ {
f := t.Field(i)
var field_dt *Datatype
field_dt, err = NewDataTypeFromType(f.Type)
var fieldDT *Datatype
Copy link
Member

Choose a reason for hiding this comment

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

ft

h5t_types.go Outdated
@@ -440,36 +440,36 @@ func NewDataTypeFromType(t reflect.Type) (*Datatype, error) {

case reflect.Struct:
sz := int(t.Size())
cdt, err := NewCompoundType(sz)
cDT, err := NewCompoundType(sz)
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this one.

@@ -110,30 +110,30 @@ func TestStructDatatype(t *testing.T) {

// Test that the type can be constructed and that the number of
// members is as expected.
var dtypes []*Datatype
var dTypes []*Datatype
Copy link
Member

Choose a reason for hiding this comment

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

types


// "Regular" value
dtype, err := NewDatatypeFromValue(test)
dType, err := NewDatatypeFromValue(test)
Copy link
Member

Choose a reason for hiding this comment

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

typ

hdf5.go Outdated
@@ -16,8 +16,8 @@ import (
func init() {
err := h5err(C.H5open())
if err != nil {
err_str := fmt.Sprintf("pb calling H5open(): %s", err)
panic(err_str)
errStr := fmt.Sprintf("pb calling H5open(): %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

Merge these lines and save the transient label.

@donkahlero
Copy link
Collaborator Author

Good comments, @kortschak! Thx!

I found one more thing that I really dislike. We have some mix-up in the test cases when it comes to fname vs. fName. I suggest we move everything to the latter?

Furthermore, I suggest that we also rename dsname in the test-cases to dsName?

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.

2 participants