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

jwnpoh · 2021-07-06T15:35:09Z

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.

- Created helper functions t.delete() and t.revert() for repetitive code.

…ted helper functions t.delete() and t.revert() for repetitive code.

…ete-command

dominikbraun · 2021-07-09T18:26:15Z

README.md

+| Argument      | Description             |
+| ------------- | ----------------------- |
+| `PROJECT KEY` | The key of the project. |


Oh wow, thanks for beautifying those tables! 😄

Honestly, I don't know anything about this! It's not possible that I had accidentally merged someone else's work when I did a git fetch upstream, is it?

My other guess is that my editor somehow interpreted these as snippets and automatically formatted the markdown tables. 😅

Honestly, I don't know anything about this! It's not possible that I had accidentally merged someone else's work when I did a git fetch upstream, is it?

No, you would have to fetch that particular contributor's fork and merge their commits into your branch - and if you do so, that contributor still appears as the author of those commits in your PR. Which editor are you using? 😄

I see, that makes sense. And day by day I see more of the genius that went into creating git!

I'm using Neovim, specifically Lunarvim which provides an IDE layer on top of Neovim.

jwnpoh · 2021-07-09T18:36:05Z

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 · 2021-07-09T18:51:42Z

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 · 2021-07-09T19:01:01Z

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 · 2021-07-09T19:03:23Z

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 · 2021-07-10T01:43:11Z

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 · 2021-07-10T07:44:53Z

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 · 2021-07-13T07:43:30Z

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 😅 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 · 2021-07-13T09:40:00Z

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 · 2021-07-18T17:19:30Z

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 · 2021-07-19T20:13:21Z

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 · 2021-07-20T17:54:18Z

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 · 2021-07-20T19:42:03Z

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.

…t new features

core/record.go

jwnpoh · 2021-07-24T17:05:45Z

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.

…ete-command

dominikbraun · 2021-08-01T20:42:15Z

cli/delete.go

@@ -44,6 +52,15 @@ func deleteProjectCommand(t *core.Timetrace) *cobra.Command {
 			key := args[0]

 			if options.Revert {
+				if !options.ExcludeRecords && askForConfirmation(revertRecordsWarning) {


Note: We may outsource this entire block into a dedicated revertProject function later.

cli/delete.go

dominikbraun

I just left some comments regarding smaller optimizations. Thanks for working on this super-powerful feature!

core/project.go

core/record.go

dominikbraun · 2021-08-02T12:13:28Z

core/record.go

+	// check for records that match project key and revert record
+	if len(records) > 0 {
+		for _, k := range keys {
+			for _, record := range records {


I think we already discussed this; ranging over all records for all projects is insanely inefficient, but there's no other way to do this ATM. We'll need to find a solution with much better runtime performance for the future.

If you have ideas for this, just open an issue (same goes for @aligator) 👍

Co-authored-by: Dominik Braun <mail@dominikbraun.io>

jwnpoh · 2021-08-02T14:40:08Z

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

Thanks for fixing those issues! I'm going to merge this large PR now, re-test everything and see if there are some things left to do. Once this is done, it will be released as part of timetrace v0.13.0!

dominikbraun · 2021-08-08T13:37:53Z

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

4d9d1b8

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

jwnpoh · 2021-08-08T15:20:49Z

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

dominikbraun · 2021-08-10T12:11:37Z

This PR is contained in timetrace v0.13.0.

jwnpoh added 5 commits July 6, 2021 22:17

delete project deletes submodules as well

fc353ee

add function to get all backup filepaths

80fe6de

- DeleteProject & RevertProject now include submodules.

33f923e

- Created helper functions t.delete() and t.revert() for repetitive code.

(i) DeleteProject & RevertProject now include submodules... (ii) Crea…

6698efe

…ted helper functions t.delete() and t.revert() for repetitive code.

Merge branch 'delete-command' of github.com:jwnpoh/timetrace into del…

2aacfbb

…ete-command

dominikbraun self-requested a review July 6, 2021 19:20

dominikbraun modified the milestones: timetrace v0.12.0, timetrace v0.13.0 Jul 7, 2021

jwnpoh and others added 4 commits July 10, 2021 02:11

add --delete-record flag and confirmation prompt for --delete-record

0ba709a

Merge branch 'dominikbraun:main' into delete-command

2f04abd

Merge branch 'dominikbraun:main' into delete-command

76dd8c1

Merge branch 'delete-command' of github.com:jwnpoh/timetrace into del…

d67e35f

…ete-command

dominikbraun reviewed Jul 9, 2021

View reviewed changes

revert to updated README

25566d0

option to restore records with --revert and update README to reflec…

3e8c73c

…t new features

jwnpoh commented Jul 24, 2021

View reviewed changes

core/record.go Outdated Show resolved Hide resolved

jwnpoh added 3 commits July 25, 2021 02:12

resolved conflics in core/record.go

d33e1a9

resolved conflics in core/record.go

a209a7c

Merge branch 'delete-command' of github.com:jwnpoh/timetrace into del…

2f7a91f

…ete-command

dominikbraun reviewed Aug 1, 2021

View reviewed changes

dominikbraun reviewed Aug 2, 2021

View reviewed changes

cli/delete.go Outdated Show resolved Hide resolved

dominikbraun reviewed Aug 2, 2021

View reviewed changes

jwnpoh and others added 3 commits August 2, 2021 22:23

Update cli/delete.go

43a15c0

Co-authored-by: Dominik Braun <mail@dominikbraun.io>

Update core/project.go

5b7f4d0

Co-authored-by: Dominik Braun <mail@dominikbraun.io>

update core/project.go and core/record.go after review

161da73

dominikbraun self-requested a review August 7, 2021 13:50

dominikbraun approved these changes Aug 7, 2021

View reviewed changes

dominikbraun merged commit 3ae698b into dominikbraun:main Aug 7, 2021

dominikbraun mentioned this pull request Aug 7, 2021

Update delete project to also remove submodules of the project #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jwnpoh commented Jul 6, 2021

dominikbraun Jul 9, 2021

jwnpoh Jul 10, 2021 •

edited

Loading

dominikbraun Jul 12, 2021

jwnpoh Jul 13, 2021

jwnpoh commented Jul 9, 2021 •

edited

Loading

jwnpoh commented Jul 9, 2021

dominikbraun commented Jul 9, 2021

dominikbraun commented Jul 9, 2021

jwnpoh commented Jul 10, 2021 •

edited

Loading

dominikbraun commented Jul 10, 2021 •

edited

Loading

jwnpoh commented Jul 13, 2021 •

edited

Loading

dominikbraun commented Jul 13, 2021

jwnpoh commented Jul 18, 2021

dominikbraun commented Jul 19, 2021

jwnpoh commented Jul 20, 2021

dominikbraun commented Jul 20, 2021

jwnpoh commented Jul 24, 2021

dominikbraun Aug 1, 2021

dominikbraun left a comment

dominikbraun Aug 2, 2021

jwnpoh commented Aug 2, 2021

dominikbraun left a comment

dominikbraun commented Aug 8, 2021

jwnpoh commented Aug 8, 2021

dominikbraun commented Aug 10, 2021

Update delete project and revert project to include submodules of the project #160

Update delete project and revert project to include submodules of the project #160

Conversation

jwnpoh commented Jul 6, 2021

dominikbraun Jul 9, 2021

Choose a reason for hiding this comment

jwnpoh Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

dominikbraun Jul 12, 2021

Choose a reason for hiding this comment

jwnpoh Jul 13, 2021

Choose a reason for hiding this comment

jwnpoh commented Jul 9, 2021 • edited Loading

jwnpoh commented Jul 9, 2021

dominikbraun commented Jul 9, 2021

dominikbraun commented Jul 9, 2021

jwnpoh commented Jul 10, 2021 • edited Loading

dominikbraun commented Jul 10, 2021 • edited Loading

jwnpoh commented Jul 13, 2021 • edited Loading

dominikbraun commented Jul 13, 2021

jwnpoh commented Jul 18, 2021

dominikbraun commented Jul 19, 2021

jwnpoh commented Jul 20, 2021

dominikbraun commented Jul 20, 2021

jwnpoh commented Jul 24, 2021

dominikbraun Aug 1, 2021

Choose a reason for hiding this comment

dominikbraun left a comment

Choose a reason for hiding this comment

dominikbraun Aug 2, 2021

Choose a reason for hiding this comment

jwnpoh commented Aug 2, 2021

dominikbraun left a comment

Choose a reason for hiding this comment

dominikbraun commented Aug 8, 2021

jwnpoh commented Aug 8, 2021

dominikbraun commented Aug 10, 2021

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

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

jwnpoh Jul 10, 2021 •

edited

Loading

jwnpoh commented Jul 9, 2021 •

edited

Loading

jwnpoh commented Jul 10, 2021 •

edited

Loading

dominikbraun commented Jul 10, 2021 •

edited

Loading

jwnpoh commented Jul 13, 2021 •

edited

Loading