~imperator/quartiermeister-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
2

[PATCH] all: Implement working hierarchical subproj print

Details
Message ID
<20210307220906.27644-1-stanner@posteo.de>
DKIM signature
pass
Download raw message
Patch: +193 -36
Suproject printing now works.
New problem: container & project are interwined too strongly.
Should be fixed soonish.
---
 bin/qm-list/main.go | 116 ++++++++++++++++++++++++++++++++++++++------
 lib/collection.go   |  85 +++++++++++++++++++++++++++++---
 lib/container.go    |  20 ++++----
 lib/project.go      |   5 +-
 qm                  |   3 +-
 5 files changed, 193 insertions(+), 36 deletions(-)

diff --git a/bin/qm-list/main.go b/bin/qm-list/main.go
index 512baca..e210538 100644
--- a/bin/qm-list/main.go
@@ -2,51 +2,135 @@ package main

import (
	"fmt"
	"os"

	qm "git.sr.ht/~imperator/quartiermeister/lib"
	"github.com/integrii/flaggy"
)

func listItems(c *qm.Collection) {
type optionsS struct {
	recurse uint
	noLines bool
}

// options should be global, otherwise get passed around 1000 times.
var (
	opts   optionsS
	gDepth int // necessary for recursive function call. Better idea welcome.
)

func listItems(c *qm.Collection) error {
	items, err := c.AllItems()
	if err != nil {
		fmt.Println("error: ", err)
		return
		return err
	}
	for _, i := range items {
		fmt.Println(i.Title)
	}

	return nil
}

func listProjs(c *qm.Collection) {
	projs, err := c.AllProjects()
func prefixByDepth(depth int) string {
	if depth == 0 {
		return ""
	}
	prefix := "├─"
	lengthener := "────"
	if opts.noLines {
		prefix = "  "
		lengthener = "    "
	}

	var longer string
	for i := 0; i < depth; i++ {
		longer += lengthener
	}
	prefix += longer

	return prefix
}

// prints passed subproject and all its subprojects recursively
func printProjAndSubs(subproj *qm.ProjectSubTreeS) error {
	var err error

	// when this function recurses, the recursors get higher and higher
	// numbers, causing the right indentation.
	prefix := prefixByDepth(gDepth)
	gDepth += 1

	// TODO: find out why there is a whitespace too much
	fmt.Println(prefix, subproj.Proj.Title)

	for _, proj := range subproj.Subprojs {
		prefix = prefixByDepth(gDepth)
		fmt.Println(prefix, proj.Proj.Title)
		subprojs, err := proj.Proj.GetSubprojectTree()
		if err != nil {
			goto out
		}

		for _, sub := range subprojs {
			gDepth += 1 // ugly... recursion.
			printProjAndSubs(sub)
			gDepth -= 1
		}
	}

out:
	gDepth -= 1
	return err
}

// loads all projects and passes them to the print function
func printProjFamily(c *qm.Collection) error {
	projs, err := c.AllProjectsN(1)
	if err != nil {
		fmt.Println("error: ", err)
		return
		return err
	}
	for _, p := range projs {
		fmt.Println(p.Title)

	for _, proj := range projs {
		tmpSubs, err := proj.GetSubprojectTree()
		if err != nil {
			return err
		}

		tmp := qm.ProjectSubTreeS{proj, tmpSubs}
		if err = printProjAndSubs(&tmp); err != nil {
			return err
		}
	}

	return nil
}

func main() {
	var (
		itemCmd = flaggy.NewSubcommand("items")
		projCmd = flaggy.NewSubcommand("projs")
	)
	var err error
	itemCmd := flaggy.NewSubcommand("items")
	projCmd := flaggy.NewSubcommand("projs")
	projCmd.UInt(&opts.recurse, "r", "recurse",
		"<N> show subprojects N layers deep")
	flaggy.Bool(&opts.noLines, "n", "no-lines", "Show no lines")

	flaggy.AttachSubcommand(itemCmd, 1)
	flaggy.AttachSubcommand(projCmd, 1)
	flaggy.Parse()

	c, err := qm.LoadCollection()
	if err != nil {
		fmt.Println("error: ", err)
		fmt.Fprintln(os.Stderr, "error: ", err)
		return
	}

	if itemCmd.Used {
		listItems(c)
		err = listItems(c)
	} else if projCmd.Used {
		listProjs(c)
		err = printProjFamily(c)
	}

	if err != nil {
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}
}
diff --git a/lib/collection.go b/lib/collection.go
index 37784ff..6a28d35 100644
--- a/lib/collection.go
+++ b/lib/collection.go
@@ -23,6 +23,11 @@ const (
	SearchAttrTag     = "tag"
)

type ProjectSubTreeS struct {
	Proj     *Project
	Subprojs []*ProjectSubTreeS
}

type Collection struct {
	Path         string
	PoolPath     string
@@ -99,6 +104,7 @@ func getProjMeta(path string) (*Project, error) {
	return proj, nil
}

// TODO: Works only properly with minimum number = 1. Maybe 0 is better?
func buildLayerNGlob(depth uint) string {
	var i uint
	s := "/"
@@ -112,34 +118,99 @@ func buildLayerNGlob(depth uint) string {
}

// get subprojects N layers deep
func (p *Project) GetSubprojectsN(depth uint) ([]*Project, error) {
func (p *Project) GetSubprojectsN(depth int) ([]*Project, error) {
	var subprojs []*Project

	glob := p.Path + buildLayerNGlob(depth)
	fmt.Println("glob: ", glob)
	glob := p.Path + buildLayerNGlob(uint(depth))
	matches, err := filepath.Glob(glob)
	if err != nil {
		return nil, err
	}
	fmt.Println("nr of matches: ", len(matches))

	for _, m := range matches {
		p, err := getProjMeta(m)
		foundP, err := getProjMeta(m)
		if err != nil {
			return nil, err
		}
		subprojs = append(subprojs, p)

		if foundP.UUID != p.UUID {
			subprojs = append(subprojs, foundP)
		}
	}

	return subprojs, nil
}

// calls itself recursively until all subprojects have been found.
func (p *Project) GetSubprojectTree() ([]*ProjectSubTreeS, error) {
	subprojs, err := p.GetSubprojectsN(1)
	if len(subprojs) == 0 || err != nil {
		return nil, err
	}

	subtree := make([]*ProjectSubTreeS, 0, 8)

	for _, subpr := range subprojs {
		deeperSubs, err := subpr.GetSubprojectTree()
		if err != nil {
			break
		}
		filler := ProjectSubTreeS{subpr, deeperSubs}
		subtree = append(subtree, &filler)
	}

	return subtree, err
}

// TODO implement this
func (c *Collection) GetProjectTree() ([]ProjectSubTreeS, error) {
	var tree []ProjectSubTreeS

	_, err := c.AllProjects()
	if err != nil {
		return nil, err
	}

	return tree, nil
}

// Gets all Projects layer N deep. Can be used to get all root projects.
func (c *Collection) AllProjectsN(depth int) ([]*Project, error) {
	var (
		projs []*Project
		err   error
	)
	if depth == 0 {
		return nil, fmt.Errorf("Invalid depth")
	} else if depth == -1 {
		return c.AllProjects()
	}

	glob := c.ProjectsPath + buildLayerNGlob(uint(depth))
	matches, err := filepath.Glob(glob)
	if err != nil {
		return nil, err
	}

	for _, m := range matches {
		foundP, err := getProjMeta(m)
		if err != nil {
			return nil, err
		}

		projs = append(projs, foundP)
	}

	return projs, nil
}

func (c *Collection) AllProjects() ([]*Project, error) {
	var (
		projs []*Project
		err   error
	)

	// FIXME: Chdir sucks
	if err = os.Chdir(c.ProjectsPath); err != nil {
		return nil, err
	}
@@ -208,6 +279,8 @@ func (p *Collection) FindTags(t *Item) ([]*Tag, error) {
	return out, nil
}

// TODO: warn user when there is more than 1 proj with identical name
// if there is, than make him choose by UUID.
func (c *Collection) ProjectByTitle(title string) (*Project, error) {
	// TODO: implement this more efficently
	projs, err := c.AllProjects()
diff --git a/lib/container.go b/lib/container.go
index 5859a42..7a32365 100644
--- a/lib/container.go
+++ b/lib/container.go
@@ -7,6 +7,7 @@ package qm

import (
	"encoding/json"
	"github.com/google/uuid"
	"os"
	"path/filepath"
	"time"
@@ -18,26 +19,24 @@ type Container struct {
	Description string    `json:"description" yaml:"description"`
	CreatedAt   time.Time `json:"created_at" yaml:"created_at"`
	Path        string    `json:"path" yaml:"-"`
	UUID        uuid.UUID `json:"uuid" yaml:"uuid"`
	parent      string
}

func checkParentExists(c *Collection, title string) error {
	_, err := c.ProjectByTitle(title)
	return err
}

// FIXME: this should not only work for Projects
func (c *Collection) CreateContainer(title,
		descr, type_, parent string) (*Container, error) {
		descr, type_, parentTitle string) (*Container, error) {
	var path string
	slug := Slug(title)

	if parent == "" {
	if parentTitle == "" {
		path = filepath.Join(c.Path, type_, slug)
	} else {
		if err := checkParentExists(c, parent); err != nil {
		parent, err := c.ProjectByTitle(parentTitle)
		if err != nil {
			return nil, err
		}
		path = filepath.Join(c.Path, type_, parent, slug)
		path = filepath.Join(parent.Path, slug)
	}

	cont := &Container{
@@ -46,7 +45,8 @@ func (c *Collection) CreateContainer(title,
		Description: descr,
		CreatedAt:   time.Now(),
		Path:        path,
		parent:      parent,
		UUID:        uuid.New(),
		parent:      parentTitle,
	}

	return cont, nil
diff --git a/lib/project.go b/lib/project.go
index 946b3d0..51b33f1 100644
--- a/lib/project.go
+++ b/lib/project.go
@@ -19,10 +19,9 @@ type Project struct {
	//Progress uint `json:"progress" yaml:"progress"`
}

func (c *Collection) CreateProject(title, description,
		parent string) (*Project, error) {
func (c *Collection) CreateProject(title, descr, parent string) (*Project, error) {
	p := &Project{}
	cont, err := c.CreateContainer(title, description, "projects", parent)
	cont, err := c.CreateContainer(title, descr, "projects", parent)
	if err != nil {
		return nil, err
	}
diff --git a/qm b/qm
index a3b2d6f..5e50a0c 100755
--- a/qm
+++ b/qm
@@ -16,10 +16,11 @@ main() {
    export QM_EDITOR="nvim"

    if [[ ! -d "$QM_PATH" ]]; then
        echo "\"$QM_PATH\" created."
        echo "No QM database yet. Iniatlizing..."
        mkdir -p "$QM_PATH"
        mkdir -p "$QM_PATH"/$QM_COLLECTION/pool
        mkdir -p "$QM_PATH"/$QM_COLLECTION/projects
        echo "\"$QM_PATH\" initialized."
    fi

    if which "qm-$cmd" >& /dev/null; then
-- 
2.20.1
Details
Message ID
<1177852a-66b5-7745-98a3-7a16b78f2693@rumpelsepp.org>
In-Reply-To
<20210307220906.27644-1-stanner@posteo.de> (view parent)
DKIM signature
pass
Download raw message
Can you create separate, smaller commits? This is difficult to review. 
Also, I am not happy with the variable names here as there are too much 
unclear suffixes and prefixes for me. Just spend a few more letters.

On 07.03.21 23:09, Philipp Stanner wrote:
> Suproject printing now works.
> New problem: container & project are interwined too strongly.
> Should be fixed soonish.
> ---
>   bin/qm-list/main.go | 116 ++++++++++++++++++++++++++++++++++++++------
>   lib/collection.go   |  85 +++++++++++++++++++++++++++++---
>   lib/container.go    |  20 ++++----
>   lib/project.go      |   5 +-
>   qm                  |   3 +-
>   5 files changed, 193 insertions(+), 36 deletions(-)
> 
> diff --git a/bin/qm-list/main.go b/bin/qm-list/main.go
> index 512baca..e210538 100644
> --- a/bin/qm-list/main.go
> +++ b/bin/qm-list/main.go
> @@ -2,51 +2,135 @@ package main
>   
>   import (
>   	"fmt"
> +	"os"
>   
>   	qm "git.sr.ht/~imperator/quartiermeister/lib"
>   	"github.com/integrii/flaggy"
>   )
>   
> -func listItems(c *qm.Collection) {
> +type optionsS struct {

optionsS?

What does the S suffix mean?

> +	recurse uint
> +	noLines bool
> +}
> +
> +// options should be global, otherwise get passed around 1000 times.
> +var (
> +	opts   optionsS
> +	gDepth int // necessary for recursive function call. Better idea welcome.

What does the g prefix mean?
Also, I don't like global variables in Go. The pollute the namespace as 
they are global per package. This makes a future refactoring quite a 
pain in the ass. Wrap them in a struct and use struct methods instead if 
required.

Also, don't use recursion. Every recursive code can be written in an 
iterative way.

> +func prefixByDepth(depth int) string {
> +	if depth == 0 {
> +		return ""
> +	}
> +	prefix := "├─"
> +	lengthener := "────"

style:

var (
     prefix     = "---"
     lengthener = "------"
)

What's a lenghtener? This variable name sounds confusing for me.

> +	if opts.noLines {
> +		prefix = "  "
> +		lengthener = "    "
> +	}
> +
> +	var longer string
> +	for i := 0; i < depth; i++ {
> +		longer += lengthener
> +	}
> +	prefix += longer
> +
> +	return prefix
> +}
> +
> +// prints passed subproject and all its subprojects recursively
> +func printProjAndSubs(subproj *qm.ProjectSubTreeS) error {
> +	var err error
> +
> +	// when this function recurses, the recursors get higher and higher
> +	// numbers, causing the right indentation.
> +	prefix := prefixByDepth(gDepth)
> +	gDepth += 1
> +
> +	// TODO: find out why there is a whitespace too much
> +	fmt.Println(prefix, subproj.Proj.Title)
> +
> +	for _, proj := range subproj.Subprojs {
> +		prefix = prefixByDepth(gDepth)
> +		fmt.Println(prefix, proj.Proj.Title)
> +		subprojs, err := proj.Proj.GetSubprojectTree()
> +		if err != nil {
> +			goto out

Don't use goto in Go code. There is e.g. defer for this. goto in go 
idiomatically is used for breaking out of nested loops, but not for 
cleanups. Despite it works (and I know the pattern from C) it is very 
uncommon and strange in Go code.

I stopped the review here, since I was overflowed with too much 
intermixed information. Can you create smaller commits for the review?

S
Details
Message ID
<8f8d4712043ce68a7952a30d4c36010cd31220d0.camel@rumpelsepp.org>
In-Reply-To
<1177852a-66b5-7745-98a3-7a16b78f2693@rumpelsepp.org> (view parent)
DKIM signature
pass
Download raw message
On Mon, 2021-03-08 at 08:23 +0100, Stefan Tatschner wrote:
> Also, don't use recursion. Every recursive code can be written in an 
> iterative way

Just saw the comment, that a better idea is welcome.

Suggestion: Create separate commits now and leave the recursion stuff
as is. We will nuke recursion in a later step once this functionality
has settled in master.

S
Reply to thread Export thread (mbox)