Skip to content

Enable initial Travis CI workflow #183

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

Merged
merged 55 commits into from
Oct 2, 2016
Merged

Conversation

peternewman
Copy link
Contributor

@peternewman peternewman commented Sep 28, 2016

And fix the resulting errors!

You'll need to turn on Travis on your repo to get the benefits, I also highly recommend the protected branch settings on GitHub, even for admins! (You can always temporarily override them as admin if you need to).

Some things to note:

  • It's not checking for unknown tags, as you're using header, article and footer in the HTML, if they were swapped for a valid tag like div or span if htmlproofer supported HTML5, we could enable that check too.
  • I think the link validation will catch people giving their org folder the wrong name, but I still need to check

What do you want to do about external links? There are a handful of dead ones currently, hence the allowed failures section on Travis.

</a>
<p class="post-summary">
<div class="post-summary">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully this doesn't break the look, but htmlproofer complains about the closing /p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if it does @Arachnid and I'll do something else instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks fine still to me.

@Arachnid
Copy link
Contributor

Oh wow, amazing job - thank you!

RE external links, it'd be good to catch people submitting invalid URLs in their submissions, but I definitely don't want to block one person's PR because another's has gone 404. In the absence of some way to do that, I'd err on the side of not validating them.

@@ -7,6 +7,6 @@ permalink: /pids/
{% for vid in vids %}
{% assign urlparts = vid.url|split:"/" %}
{% assign vidno = urlparts[1] %}
<h3><a href="{{vid.url}}">0x{{urlparts[1]}}</a></h3>
<h3><a href="{{vid.url}}">0x{{vidno}}</a></h3>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is still generating a pair of p /p around it, which the validator doesn't like. Any thoughts on how to get rid of it @Arachnid ? I wondered if switching the whole line to markdown might work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. I suspect it's the empty lines from the assign statements, which markdown is treating as empty paragraphs. Maybe put it inside a div or try moving them all onto one line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manage to fix it by just removing the leading spaces.

@peternewman
Copy link
Contributor Author

No worries.

I'll have a ponder about external links. There is some caching stuff regarding them and I think you can ignore URLs, so you could ignore the broken ones; it's not perfect, but may be an acceptable compromise.

Alternatively you can manually refer to the allowed failures section of Travis.

@peternewman
Copy link
Contributor Author

Can you click the button here to turn on Travis please @Arachnid https://travis-ci.org/pidcodes/pidcodes.github.com/ ?

@Arachnid
Copy link
Contributor

Of course. Sorry, I thought that was only useful after submitting this PR.

@peternewman
Copy link
Contributor Author

I hoped it would, and indeed after a new commit it's started showing the PR builds: https://travis-ci.org/pidcodes/pidcodes.github.com/builds/164354824 !

@peternewman
Copy link
Contributor Author

Given this is now green, I'd be tempted to merge it now if you're happy, and I can look at addressing the other bits going forward, but in the meantime you'll already get the benefits of some initial Travis testing being in place.

@Arachnid
Copy link
Contributor

Arachnid commented Oct 2, 2016

Indeed I am - thanks again for all the hard work!

@Arachnid Arachnid merged commit 3706846 into pidcodes:master Oct 2, 2016
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