Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New build passing badge #142

Merged
merged 2 commits into from
Nov 9, 2020
Merged

New build passing badge #142

merged 2 commits into from
Nov 9, 2020

Conversation

didriklundberg
Copy link
Member

I added a new CI badge to the top. This looks nice, I think.

@didriklundberg
Copy link
Member Author

@andreaslindner , could you take a look at the reason for the below when you have time:

[[ "$(head -n1 "$1/README.md" 2> /dev/null | grep -c '# HolBA')" -eq 1 ]] \
|| { echo "bad README.md" > /dev/stderr; return $FALSE; }

Ironically, this commit caused the build to fail due to this check.

@andreaslindner
Copy link
Member

andreaslindner commented Nov 9, 2020

I didn't add the badge because it is almost non-sense. Browsers cache these images and thus a failing check (because of the weird deadlocks that sometimes happen for example) may stay preserved for very long. Likewise, a successful check may be preserved.

As I see it this is a non-sense feature that everybody adds. Especially since it is added by more maintained projects that keep it always passing. The information content of this is near zero and because many add it, it has kind of a status value.

That being said, I am not against if you want to have it.

You can simply change the check in HolBA/scripts/setup/env_config_gen.sh in such a way that it still validates in a meaningful way that it indeed found the root directory of HolBA but works with the new changes. It doesn't need to check that the first line of the README starts with # HolBA. This was just a nice extra factor adding to the previous ones in this file.

@didriklundberg
Copy link
Member Author

I changed the position of the badge, which made the "bad README" check pass. I've seen both positions in other repositories, I think this also looks nice.

I was going through some earlier issues earlier and did this very quick PR in response to #103, I didn't recall any opinions on this matter from previously.

Personally, I think the README should look nice and be useful for visitors, so even if things don't make a lot of sense for us (we know builds on main branch pass CI) it might still serve a purpose in the README file. There are also some other badges we might consider adding later, like for example the Zulip chat link if we should choose to migrate to that platform.

@andreaslindner
Copy link
Member

Yes, that could be also useful if we plan to have a channel for the general public or similar.

Copy link
Member

@andreaslindner andreaslindner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@didriklundberg didriklundberg merged commit f186d79 into master Nov 9, 2020
@didriklundberg didriklundberg deleted the dev_readme_bp_badge branch November 9, 2020 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants