Update `delete project` and `revert project` to include submodules of the project

[jwnpoh]

This PR addresses #74.

I thought that if we delete submodules by default, it would make sense if --revert would also revert submodules, and so created additional functions for that even though that was not asked for. If you think --revert should not revert submodules by default, let me know and I'll be happy to amend the commit.

[jwnpoh]

Sorry for the numerous commits!!! I am still figuring out git...

The latest commits are WIP addressing #161. The revert project function still needs some work, such that it also reverts deleted project records. Currently, I have implemented the option to delete project records in cli/delete.go, by calling some new functions that I added to core, but I would like to seek your review of whether this is a good way to do it.

The main challenge that I had was retrieving the records for the submodules in addition to the records for the parent project (since we are now also deleting submodules by default, the option to include records in the delete should also include the submodule records). I wonder if there's a better way to do it than the nested for loops in DeleteRecordsByProject in core/record.go?

[jwnpoh]

I would greatly appreciate some feedback on whether I've been going about this correctly, before proceeding to work on the updates to the revert project function.

Thanks!

[dominikbraun]

The main challenge that I had was retrieving the records for the submodules in addition to the records for the parent project (since we are now also deleting submodules by default, the option to include records in the delete should also include the submodule records). I wonder if there's a better way to do it than the nested for loops in DeleteRecordsByProject in core/record.go?

Deleting the records of the submodules along with the records of the actual project makes sense and is consistent, so this is fine. I'm afraid there isn't a better way to delete the records other than using those nested loops. There might be some tricky performance tweak using a map to solve this more efficiently, but I don't see an easy option at the moment.

I would greatly appreciate some feedback on whether I've been going about this correctly, before proceeding to work on the updates to the revert project function.

I think you're solving this correctly and just as expected!

The only thing I'm getting more and more concerned of is that the Timetrace struct and the Fs interface are growing and growing, doing a lot of different things - but this is an issue to be addressed later.

[dominikbraun]

BTW, I moved this PR from milestone v0.12.0 to v0.13.0 since the 0.12.0 release got pretty bloated. I'm going to release 0.12.0 with some minor changes, e.g. two new flags. Once those things are cleaned up, I'm going to release 0.13.0 with those more impactful features.

[jwnpoh]

I think you're solving this correctly and just as expected!

Thanks, glad to know that I'm on the right track!

The only thing I'm getting more and more concerned of is that the Timetrace struct and the Fs interface are growing and growing, doing a lot of different things - but this is an issue to be addressed later.

With respect to this PR, I added one method ProjectBackupFilepaths to the Fs interface. When thinking about it I wondered about an alternative approach which was to modify the existing ProjectFilePaths method to take a filter argument, something like ProjecttFilePaths("all" | "active" | "bak"). Is there any merit to such an approach? I think it probably makes the overall code less readable/maintainable, though the alternative, as we have now, means a big Fs struct/interface.

BTW, I moved this PR from milestone v0.12.0 to v0.13.0 since the 0.12.0 release got pretty bloated. I'm going to release 0.12.0 with some minor changes, e.g. two new flags. Once those things are cleaned up, I'm going to release 0.13.0 with those more impactful features.

That's just as well; I will take a little bit more time to work on the remaining revert functions as I have quite a bit more work to clear from my paying job these days!

I'd also just like to say thanks again for this project. I'm finding good utility for it as a user and good learning from working on the code as well.

[dominikbraun]

With respect to this PR, I added one method ProjectBackupFilepaths to the Fs interface. When thinking about it I wondered about an alternative approach which was to modify the existing ProjectFilePaths method to take a filter argument, something like ProjecttFilePaths("all" | "active" | "bak"). Is there any merit to such an approach? I think it probably makes the overall code less readable/maintainable, though the alternative, as we have now, means a big Fs struct/interface.

Creating a more generic function that takes filter arguments is a nice idea. "all" | "active" | "bak" is not possible from a syntactical point of view, you need to introduce constants for this. The cleanest approach would be something like this:

type ProjectType int

const (
    All    ProjectType = 0
    Active ProjectType = 1
    Bak    ProjectType = 2
)

func ProjectFilepaths(projectTypes ...ProjectType) []string {
    for _, project := range myProjects {
        for _, projectType := range projectTypes {
            // ... Filter by project type
        }
    }
}

// Calling the function:

paths := ProjectFilepaths(Active, Bak)

But I'm going to create a separate issue for this.

[jwnpoh]

const (
    All    ProjectType = 0
    Active ProjectType = 1
    Bak    ProjectType = 2 
)

Yes this is what I was thinking but didn't know how to express quite as clearly in code, didn't think to just write out a code snippet :sweat_smile: Is this what is known as an enum in other languages?

Is this const declaration block possible to do using iota? And would that be equivalent to simply assigning integer values to each constant?

[dominikbraun]

Is this what is known as an enum in other languages?

Yes, they're the equivalent to enumerations.

Is this const declaration block possible to do using iota?

We even should do it with iota, but I didn't want to cause confusion in case you don't know iota yet 😄

And would that be equivalent to simply assigning integer values to each constant?

Yes, since iota starts at 0 and is increased for each subsequent constant.

[jwnpoh]

Hello, in my last commit, revert project reverts the parent project and submodules. I think, logically, revert project should also revert records, right?

Question 1: Does there even need to be an option here, or can we assume that revert project definitely also means revert records?

I think the answer may not be so straightforward as illustrated in the following scenario:

• user maybe has deleted make-coffee but decided to keep the records
• user makes edits to the records previously under make-coffee
• user decides to restore make-coffee, but with the new edits preserved

(I don't know why anyone would do things this way but it is a possibility.)

Question 2: We could make assumptions/make timetrace opinionated and just dictate that revert project will revert records, and presume that user wants to preserve any possible new edits that were made after the project had been previously deleted. Implementing this, my current idea (haven't tried it out yet) is to use fs.FileInfo.ModTime() and compare between the .json and .json.bak files of the same record. Something like:

recordBaks := getAllBackupRecords()
for _, recordBak := range recordBaks {
    if record.ModTime() > recordBak.ModTime() {
        continue
    }

    revert(recordBak)
}

Question 3: We could give the user options. However, since revert is not a command but a flag. I am not sure how we might implement the options. Do we do it through interactive prompts after the user has entered timetrace delete project make-coffee -r? Or through flags?

If through flags, would it make sense to add a new flag, for eg -c / --revert-records or make use of the existing one in my last commit (not yet merged)? For eg., to delete records with the project, the user can pass the -d flag. To revert records along with reverting project, the user will pass both -d and -r flags like so: timetrace delete project make-coffee -r -d.

[dominikbraun]

Restoring the records isn't a must-have requirement for me, but a nice-to-have for sure.

1) Your edge case is a good point. In that case, it indeed would make sense to require a flag for restoring the records and ask for confirmation, warning that the edited records will be overwritten - except 2) would actually work, which would be a bit crazy but powerful 😄

3) I'm not sure on this yet. Maybe it would make sense to introduce a revert command as the inverse operation of edit and delete? 🤔

[jwnpoh]

3) I'm not sure on this yet. Maybe it would make sense to introduce a revert command as the inverse operation of edit and delete?

When I first came across the revert functions I had initially wondered why these were not implemented as a separate Cobra command. After a while, it seemed to me that maybe the intent of the --revert is to be a simple undo action. And so that made sense to me. revert is context-dependent, being only relevant to delete and edit. Seen this way, I think it makes sense for revert to remain a flag rather than a command.

After thinking about this problem a little more I think I may have a clean solution, and would love to hear your thoughts on this:

• Reverting of records along with the project and submodules is the default behaviour of revert.
• Should this default behaviour not be to the user's liking, the user can pass in a flag to opt out of reverting records eg. timetrace delete project make-coffee --revert --exclude-records.
• If the flag is not present, the command will give a warning that records will be reverted from backup and any new changes to record will be overwritten, and prompt the user for confirmation.
• If confirmation is not given, the program exits without reverting anything and hints the user to use the --exclude-records flag.
• Else, revert everything - project, submodules, records.

[dominikbraun]

Your ideas absolutely make sense 👍 I think flag combinations like --revert --exclude records are indeed the only clean solution if --revert remains a flag. On the other hand, if revert was a command, resources (projects, modules and resources) would have sort of a lifecycle: create, edit, (revert), delete, revert - which is an interesting thought, too.

[jwnpoh]

Hello! deleteProjectCommand now has the --exclude-records or -e flag available. This flag modifies the behaviour of both timetrace delete project as well as timetrace delete project --revert.

If the -e flag is passed, the project will be deleted/restored without doing anything to records. If no -e flag is passed, an additional prompt will ask the user for confirmation on whether to delete / revert project records. If the user inputs n, it will be equivalent to -e.

[jwnpoh]

Thanks for the review! In particular I learnt something about the behaviour about the for loop. I had previously thought that looping over an empty slice would throw an error, hence the inclusion of the if check.

I have added a new commit implementing the suggested changes.

[dominikbraun]

@jwnpoh So this will be included in timetrace v0.13.0:

https://github.com/dominikbraun/timetrace/commit/4d9d1b8cbf8cebce4df18b235b18ec3417fef130

Did I miss anything in the Added or Changed sections regarding this PR? 😄

[jwnpoh]

@dominikbraun looks about right. Don't think you've missed anything!

[dominikbraun]

This PR is contained in timetrace v0.13.0.