task icon indicating copy to clipboard operation
task copied to clipboard

refactor: Re-organize node loading code to make it easier to follow

Open pbitty opened this issue 1 year ago • 3 comments

While working in this part of the code I found it difficult to follow the different code paths when loading a node's content. I realize readability is subjective, but I am suggesting this change in case you also find it improves readability and maybe maintainability.

In this change I am attempting to make the branches of the decision tree return early as much as possible. This way we discard each possibility as we work our way down, in this order:

  1. Non-remote nodes
  2. In offline mode, use cached remote nodes
  3. On network timeout, use cached remote nodes
  4. a. Use fetched remote node b. On checksum change, handle writing to cache

Tests are added to ensure this functionality does not break.

I broke up the changes into logical commits that are fairly isolated. It might be easier to review them individually.

What do you think?

(I got into this code path while exploring how to incorporate go-getter into the remote taskfile feature (as proposed in this comment). Thanks for maintaining this tool!)

pbitty avatar Aug 21 '24 21:08 pbitty

Hi @pbitty!

I like the idea, but there a couple of thing that needs to be addressed before merging:

Thanks, will do!

  1. Tests are failing unless TASK_X_REMOTE_TASKFILES=1 is set

Ha, I missed this because I have set it permanently in my shell environment. I figure the best place to add it is in the Go tests themselves. Let me know if you have another idea.

pbitty avatar Aug 26 '24 14:08 pbitty

@andreynering , I did a force-push in order to alter the commit messages. Do they look ok?

I could also squash all the commits down to a single one, and call it

refactor: Re-organize node loading code to make it easier to follow

or something along those lines.

pbitty avatar Aug 26 '24 15:08 pbitty

Hey @pbitty ! Thanks for your contribution! Like @andreynering, I like the idea, I find this more readable. Can you also modify the PR title to follow Conventional Commits? because we will squash and merge.

I would like to merge https://github.com/go-task/task/pull/1713 first as it solves one issue (Timeout message is not correct) What is your opinion about it @andreynering ?

vmaerten avatar Aug 26 '24 16:08 vmaerten

Just noting that I haven't forgotten about this, just trying to find some time this week to do it.

pbitty avatar Oct 10 '24 17:10 pbitty

@andreynering , @vmaerten, the rebase is done! Ready for merge.

The Compare link above shows 94 changed files, but you can see the ones that had conflicts if you click on the files themselves. Here are the links if it helps:

I added one more commit which uses math/rand/v2 to make the tests I added a little bit cleaner.

pbitty avatar Oct 17 '24 21:10 pbitty

Thanks!

vmaerten avatar Oct 18 '24 16:10 vmaerten