Cleanup verify #2043

Merged
merged 1 commit into from Oct 16, 2016

Projects

None yet

4 participants

@stephengroat
Member
stephengroat commented Sep 16, 2016 edited

This is an idea.

I enabled rubocop, which is an aggressive ruby linting tool. you can see that i disable a few of the check for the of the more complex functions, but this does force uniformity in the ruby code.

adding a .rubocop.yml is also an option (instead of the inline # rubocop:disable comments)

general comments or hatred welcome

next goal is to get/make a linting tool that tools at the editorconfig for all files

@psgs psgs added the enhancement label Sep 17, 2016
@mxxcon
Member
mxxcon commented Sep 17, 2016

So what's the advantage of this over what we already have?

@Carlgo11
Member

@mxxcon it's just a cleanup. No advantages more than that the code looks nicer

On Sep 17, 2016, at 00:09, mxxcon notifications@github.com wrote:

So what's the advantage of this over what we already have?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@mxxcon
Member
mxxcon commented Sep 17, 2016

But we now have a new dependency on rubocop gem, don't we?

@stephengroat
Member

adds a gem dependency only for test. my goal is to add a lot more to the test

  1. figure out more extensive tests to run just on sections updated by a git pull (url checking etc)
  2. get an internal section that uses apis (twitter, facebook, domaintally). since the only PRs that are made by members can use API keys, maybe start creating an internal PR from master to gh-pages to trigger travis to do those

i think it would be a a great goal to automate ourselves out of existence

@Carlgo11
Member

psst, rebase when you're done 😉

@stephengroat
Member
stephengroat commented Sep 18, 2016 edited

will do. geez, it's like the guy trying to commit stuff for code cleanliness should have clean commits. seems like a lot of work ...

@Carlgo11

The code looks nice but I don't know how good of an idea it is to introduce yet another way the build can fail.
My view on the build statuses is that checks should be run for things essentials for the project but not for cosmetic purposes only.

Maybe instead of adding the rubocop gem you could use this PR to tidy up the verify.rb script?

@stephengroat
Member
stephengroat commented Sep 19, 2016 edited

I agree it's dependency/another way for the build to fail, but I'm not sure I agree with:

My view on the build statuses is that checks should be run for things essentials for the project but not for cosmetic purposes only.

Why not check whatever pieces can be automatically checked? I would think that the eventual goal of this project would be that every piece of a PR is checked during the CI process and only minimal human review is needed (i.e. i saw that previous URL checking produced timeouts during the build process, but what if only the URLs affected by the PR were checked (reducing the URL checking surface area)).

P.S. i don't mean to be an ass

@Carlgo11
Member

With cosmetic purposes I mean things like formatting and white space. URL checking is not something cosmetic.

Making the code look nice is a good thing but the build shouldn't fail just because of formatting.

@Carlgo11 Carlgo11 closed this Sep 20, 2016
@Carlgo11 Carlgo11 reopened this Sep 20, 2016
@Carlgo11
Member

(Accidentally closed the PR. Opened it again 😅)

@stephengroat
Member

I we don't allow PRs if they remove trailing whitespace, shouldn't that be part of the automated checks?

@psgs
psgs approved these changes Oct 16, 2016 View changes
@psgs psgs added the looks-good label Oct 16, 2016
@YesThatAllen YesThatAllen referenced this pull request Oct 16, 2016
Merged

Misc maintenance #2127

@Carlgo11 Carlgo11 merged commit 9e8b7e3 into 2factorauth:master Oct 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stephengroat stephengroat deleted the stephengroat:cleanup-verify branch Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment