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.

Sign up for GitHub

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

Jump to bottom

Added list command #5

Merged
merged 6 commits into from May 22, 2017
Merged

Added list command #5

merged 6 commits into from May 22, 2017

Conversation

adelowo
Copy link
Contributor

@adelowo adelowo commented May 20, 2017

This is a fix for go-aah/aah#42 ....

As an aside, is there a way tests can be written for this cli tool ? I recommend moving to something much more fine grained where it would be pretty easy to do something of that sort

jeevatkm
jeevatkm requested changes May 21, 2017
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm 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 the PR. Can you please have a look on requested changes?

aah cli is same as Go command. Typically sub commands with own set of flags in it. I believe it is unit testable. As of now I have not written one. I will check the go command unit test cases and write one for aah cli.

I recommend moving to something much more fine grained

Can you please shed some light? what you have in your mind? I would like to know.

aah/list.go Outdated

func listRun(args []string) {

gopath, err := ess.GoPath()
Copy link
Member

Choose a reason for hiding this comment

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

gopath is already validated at aah.go, Can you please remove L#31 to L#36?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops.. I didn't see that

aah/list.go Outdated

var count int

filepath.Walk(gopath, func(path string, info os.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update this to ess.Walk? It is improved version of standard filepath.Walk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would do that... I didn't review the essentials package enough

aah/list.go
}

if info.IsDir() {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Please use this one return filepath.SkipDir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If SkipDir is returned, the entire content of that directory would be skipped (https://golang.org/pkg/path/filepath/#WalkFunc).. And since the file path is being inspected for aah.project, the command wouldn't work..

Copy link
Member

Choose a reason for hiding this comment

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

Okay my bad. please keep return nil.

Copy link
Contributor Author

@adelowo adelowo May 22, 2017 •
edited

Choose a reason for hiding this comment

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

Done.. It was actually left there in the latest commit

aah/list.go Outdated

if isAahProject(path) {
count++
fmt.Println(filepath.Dir(path))
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add the path info into string slice and print it after L#60?

aah/list.go Outdated
if isAahProject(path) {
count++
fmt.Println(filepath.Dir(path))
return nil
Copy link
Member

Choose a reason for hiding this comment

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

I believe, this is return statement is not needed.

aah/list.go Outdated

}

func isAahProject(dir string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I think we are passing file as param, not a directory. Shall we rename variable to file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jeevatkm
Copy link
Member

jeevatkm commented May 21, 2017 •
edited

@adelowo Found the references to writing unit test for go cli program.

  1. About TestMain
  2. go_test.go

Thanks.

@jeevatkm jeevatkm mentioned this pull request May 21, 2017
Closed
@adelowo
Copy link
Contributor Author

adelowo commented May 22, 2017

Sorry for the late response, I had a bad weekend ...

@jeevatkm
Copy link
Member

Don't worry, take your time. I hope you're doing well now. Thanks again for the PR.

Once your changes is done, push it. I have a look and merge it.

Removed duplication, added better naming and made use of the essential
9ab6b05 
pkg rather than filepath from the standard lib
@adelowo
Copy link
Contributor Author

adelowo commented May 22, 2017

Thanks @jeevatkm . I am better now.

As for this comment I recommend moving to something much more fine grained. I meant something like a dedicated cli library that (hopefully) would make writing tests easy enough. But I would go through those links and see if it is workable

@jeevatkm
Copy link
Member

It seems you have incorporated all the requested changes. Please give me sometime. I will run it locally and get back to you if any improvements needed.

Let's make it good together.

@adelowo
Copy link
Contributor Author

adelowo commented May 22, 2017

No problem @jeevatkm

@jeevatkm
Copy link
Member

@adelowo I have verified locally. It is working fine. Presentation needs to be improved, Windows OS compatibility and it's related code changes.

I will do it after merging your PR. Thank you.

Following is the goal to achieve:

––––––––––––––––––––––––––––––––––––––––––––––––––––
   aah framework v0.5.1 -  https://aahframework.org
––––––––––––––––––––––––––––––––––––––––––––––––––––

Scanning GOPATH:  /Users/jeeva/go-home/...

4 aah projects were found, import paths are-
      github.com/go-aah/tutorials/curd
      github.com/go-aah/website
      testappscope
      testapp1

@jeevatkm jeevatkm merged commit 386b1aa into go-aah:v0-unstable May 22, 2017
@jeevatkm jeevatkm modified the milestone: v0.6 Milestone May 22, 2017
@adelowo adelowo deleted the add_list_command branch May 22, 2017 23:41
jeevatkm added a commit that referenced this pull request May 23, 2017
@jeevatkm
go-aah/aah#42, #5 presentation done for list cmd
1dfe009
@jeevatkm jeevatkm mentioned this pull request Jun 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeevatkm jeevatkm jeevatkm approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v0.6 Milestone
2 participants
@adelowo @jeevatkm