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

[RFC] Feature #26 export to ical #27

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

Conversation

NMFR
Copy link
Collaborator

@NMFR NMFR commented Jan 9, 2019

No description provided.

golog.go Outdated Show resolved Hide resolved
golog.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter_test.go Outdated Show resolved Hide resolved
@NMFR NMFR force-pushed the feature-26-export-to-ical branch 2 times, most recently from 534913f to 4582c34 Compare January 10, 2019 09:33
Copy link

@jsaguiar jsaguiar left a comment

Choose a reason for hiding this comment

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

/Users/joao/Desktop/golog/golog.go:15: exported const ExportICal should have comment (or a comment on this block) or be unexported
/Users/joao/Desktop/golog/golog.go:18: exported var ExporterNames should have comment or be unexported
/Users/joao/Desktop/golog/ical_exporter.go:5: exported const PRODID should have comment or be unexported
/Users/joao/Desktop/golog/ical_exporter.go:7: exported type ICalTaskExporter should have comment or be unexported
/Users/joao/Desktop/golog/ical_exporter.go:83: exported method ICalTaskExporter.Export should have comment or be unexported
/Users/joao/Desktop/golog/ical_exporter.go:121: exported method ICalTaskExporter.GetFileExtension should have comment or be unexported
/Users/joao/Desktop/golog/tasks.go:4: exported const TaskStart should have comment (or a comment on this block) or be unexported

golog.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
golog.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
golog.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
ical_exporter.go Outdated Show resolved Hide resolved
To export run:
```sh
golog export
```

Or:
```sh
golog export ical golog.ics
```

Closes mlimaloureiro#26
@NMFR NMFR force-pushed the feature-26-export-to-ical branch 4 times, most recently from b98ada7 to 5b7a65b Compare January 16, 2019 16:42
Version 1.1.2 is being removed due to and error on the github.com/codegangsta/cli@v1.20.0 dependency:
````
../../codegangsta/cli/context.go:170: undefined: flag.Getter
```

Seems codegangsta/cli is using go's flag.Getter that was only [added in go v1.2](golang/go@49b3301)

Other versions were replaced by 1.11.4 because the project started using go modules
@NMFR NMFR changed the title Feature #26 export to ical [WIP] Feature #26 export to ical Jan 16, 2019
Add SetTask and SetTasks methods to TaskRepositoryInterface
Implement the new TaskRepositoryInterface methods
Add csv export format to the CLI
Add comments to public structs / methods
@NMFR NMFR changed the title [WIP] Feature #26 export to ical [RFC] Feature #26 export to ical Jan 18, 2019
models/tasks/activity_test.go Outdated Show resolved Hide resolved

t.Run("not running", func(t *testing.T) {
taskActivity := TaskActivity{StartDate: timeFromString("2019-01-01T10:00:01Z"), EndDate: timeFromString("2019-01-02T10:00:00Z")}
assert.Equal(t, 24*time.Hour-1*time.Second, taskActivity.Duration())
Copy link
Owner

Choose a reason for hiding this comment

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

add space between operators for readability:

24 * time.Hour - 1 * ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

go fmt removes the spaces there.

// TaskActivity represents a date interval of a task execution
type TaskActivity struct {
StartDate time.Time
EndDate time.Time // EndDate.IsZero() indicates a running task, meaning the 0001-01-01 EndDate cannot be represented
Copy link
Owner

Choose a reason for hiding this comment

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

should we have in line comments, or comments in the previous line ? let's also add this to the standards if we don't have it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we have to pick only one i would vote for comments in the previous line. Helps keeping line length low and provides a explanation / comment before the actual code

But do we really need to pick only one? Sometimes i think it makes sense inline, like in this occurrence where it seems to make more sense to have the comment explaining the meaning of EndDate.IsZero() after EndDate has been declared.

models/tasks/activity_test.go Outdated Show resolved Hide resolved
models/tasks/activity_test.go Outdated Show resolved Hide resolved
models/tasks/activity_test.go Outdated Show resolved Hide resolved
package tasks // import github.com/mlimaloureiro/golog/repositories/tasks

// DeleterTaskRepositoryInterface is a TaskRepositoryInterface that can also delete tasks directly
type DeleterTaskRepositoryInterface interface {
Copy link
Owner

Choose a reason for hiding this comment

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

what was the reason to have a dedicated interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some repositories might have implementations for this 2 operations (for example a SQL repository). This interface would allow TaskService to call those methods directly instead of using GetTasks() -> SetTasks() to delete tasks. Something similar to StarterPauserTaskRepositoryInterface.

But currently no repositories implement it.

Should i remove it?

repositories/tasks/file/csv/repository_test.go Outdated Show resolved Hide resolved
func getFileContent(t *testing.T, filePath string) string {
stringBytes, err := ioutil.ReadFile(filePath)
assert.NoError(t, err)
return string(stringBytes)
Copy link
Owner

Choose a reason for hiding this comment

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

always add a new line in the last return, let's also add it to our styleguide

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean like this:

func getFileContent(t *testing.T, filePath string) string {
	stringBytes, err := ioutil.ReadFile(filePath)
	assert.NoError(t, err)
	
	return string(stringBytes)
}

?

When there is only one line this should be ignored right?

func GetPi() float64 {
    return 3.14
}

repositories/tasks/memory/repository.go Outdated Show resolved Hide resolved
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.

None yet

3 participants