2: Remove version

[gabyx]

Not yet ready.

[coveralls]

Pull Request Test Coverage Report for Build 691

• 18 of 22 (81.82%) changed or added relevant lines in 3 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.1%) to 81.67%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
install.sh	11	15	73.33%
<!--	Total:	18	22	81.82%	-->

Files with Coverage Reduction	New Missed Lines	%
install.sh	1	80.12%
cli.sh	2	82.92%
<!--	Total:	3		-->

Totals
Change from base Build 687:	0.1%
Covered Lines:	2103
Relevant Lines:	2575

---

💛 - Coveralls

[gabyx]

The tests listed here habe some Version resetting which did not get ported to a git reset unfortunately. Do you understand why these test still worked :)? I think because you only test the autoupdate.lastrun variable which is not really a test about if the update was executed but only if the check was executed, was that the intend? i think we rather should concentrate on having a few tests but proper ones. I am not sure about that. What do you think and were the tests intended for something else ?

[rycus86]

I think the previous code was only setting that variable after a successful update if I remember correctly, perhaps that's why it worked in the first place. Not sure why the new code made it still pass, have a look at https://coveralls.io/github/rycus86/githooks if you're interested, maybe the coverage sheds a light on which paths are tested since the update mechanism changed.

i think we rather should concentrate on having a few tests but proper ones. I am not sure about that.

I think we could test more, but didn't want to ask you to do that on top of all the great work you're doing here already. I'm not too concerned for now as we don't get too many bugs here, most of them are found by you pretty quickly if they slip through, so that's OK I think. If it gets hard to make changes because things break without tests, then we should revisit this. On the other hand, I wouldn't start deleting tests just yet, they were covering all of the functionality more or less, which didn't change massively, only their implementation. I might have written some tests badly though that assumed too much about the internals, which is we those might give you trouble now. 🙈

[gabyx]

Hm.. sound good to me: The question is the

check_for_updates_if_needed() {
    read_last_update_time
    should_run_update_checks || return
    record_update_time <<<<<<<<<<<<<<<<<<<<HERE
    fetch_latest_updates || return
    should_run_update && execute_update && return
    print_update_disable_info
}

I cannot remember that thing changed the order? Currently, this is before any update happened? Should that be moved after the update really took place? or is lastrun only for denoting the last check for update triggern?

[gabyx]

Ok, from what I read in read_last_update_time its just the update check time...

[rycus86]

Yeah, I can't remember exactly now. From what you pasted here, it does look like it was maybe testing the wrong thing. With the new update perhaps comparing commits is better, I think you've done this already in a few tests.

[gabyx]

As I found out: https://unix.stackexchange.com/questions/591618/how-to-supress-the-error-of-file-redirection

read $VARIABLE </dev/tty would have been enough since that nasti tty cannot open output is still there :-) So long story short it would have been easier than we discussed...

if true 2>/dev/null </dev/tty; then
        # shellcheck disable=SC2229
        read -r "$VARIABLE" </dev/tty
    fi

[gabyx]

I removed the VERSION file completly since its not needed, if we would keep it we only have merge troubles always which could be resolved by adding a git merge driver which is kind of overkill for that we dont need the VERSION file anyway. We look up the version in the cli.sh and elsewhere for the output with git rev-parse. If we would introduce some tags with could use git describe which is nice :-).

With that PR we cannot see the version anymore in the installed base-template.sh. But hopefully that will drop, maybe we can write the version number to the wrapper script with a post-commit hook, whenever the wrapper is changed... later

[gabyx]

Output

[gabyx]

By the way you should try oh-my-zsh (if you're not already useing it) and https://github.com/denysdovhan/spaceship-prompt and code highligthing which is really neat, you make less errors :-) in the shell https://computingforgeeks.com/how-to-configure-zsh-syntax-highlighting-on-linux-macos/#:~:text=Zsh%20syntax%20highlighting%20enables%20highlighting,particularly%20in%20catching%20syntax%20errors.&text=Syntax%20highlighting%20is%20done%20by%20pluggable%20highlighter%20scripts.

[rycus86]

LGTM, please check the two comments above, then we should be good to merge. 👍

[rycus86]

Also wanted to say: thank you very much for these small PRs, much easier to review them! 👌🙂

[gabyx]

Rebased

[gabyx]

Wait I rsquash.

[gabyx]

I made some major errors in the legacy transformations typo : githoooks ->Shit thats why we would need a dev branch =)

[gabyx]

Fixed the legacy transformation bug too. Merge asap into master. Thx.

[gabyx]

Good to go, all tests pass

[gabyx]

@rycus86 Sorry to bother, could you merge this, because this fixes also the legacy_transformation crap I ve written -> lots of bad typos...

[rycus86]

I made some major errors in the legacy transformations typo : githoooks ->Shit thats why we would need a dev branch =)

Or perhaps more tests to make sure new code does what we want it to do. ;)

[rycus86]

I've added a commit to try and be more consistent with the version number display. I've also fixed the Arch tests. :partying_face: Not sure what's wrong with ls there, but find -type f worked instead. :man_shrugging: