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

Hide comments from specific users [issue #197] #329

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@blueyedgeek

blueyedgeek commented Nov 25, 2016

This PR implements the feature requested in #197

@blueyedgeek blueyedgeek changed the title from Hide comments from specific users [issue 197] to Hide comments from specific users [issue #197] Nov 25, 2016

@crertel

This comment has been minimized.

Show comment
Hide comment
@crertel

crertel Nov 26, 2016

Contributor

Some questions:

  • Why did you add a dependency on pg that wasn't there before, and why is the PG extension now a hard requirement, when the current codebase uses MySQL?
  • Why did you add a dependency on addressable that wasn't there before?
  • Why did you remove the limits on the text columns (and what does this mean for people submitting maliciously large data)?
  • Why did you increase the limit on the vote column in the vote table?

Good testing though! Just maybe try to stick with the idioms already present in the codebase.

EDIT: removed question about capybara--now I see what you were up to there.

Contributor

crertel commented Nov 26, 2016

Some questions:

  • Why did you add a dependency on pg that wasn't there before, and why is the PG extension now a hard requirement, when the current codebase uses MySQL?
  • Why did you add a dependency on addressable that wasn't there before?
  • Why did you remove the limits on the text columns (and what does this mean for people submitting maliciously large data)?
  • Why did you increase the limit on the vote column in the vote table?

Good testing though! Just maybe try to stick with the idioms already present in the codebase.

EDIT: removed question about capybara--now I see what you were up to there.

@blueyedgeek

This comment has been minimized.

Show comment
Hide comment
@blueyedgeek

blueyedgeek Nov 28, 2016

Why did you add a dependency on pg that wasn't there before, and why is the PG extension now a hard requirement, when the current codebase uses MySQL?

An oversight on my part (I'm most familiar with postgres and I used it instead of mysql). I'll update the pr and remove the dependency on the postgres gem

Why did you add a dependency on addressable that wasn't there before?

It's a runtime dependency for Capybara (added by the capybara gem)

Why did you remove the limits on the text columns (and what does this mean for people submitting maliciously large data)?
Why did you increase the limit on the vote column in the vote table?

This I got to look into as I only added the migration to create one new table (blocked users) and I am unclear as to why this would cause the limit constraints to be removed.

blueyedgeek commented Nov 28, 2016

Why did you add a dependency on pg that wasn't there before, and why is the PG extension now a hard requirement, when the current codebase uses MySQL?

An oversight on my part (I'm most familiar with postgres and I used it instead of mysql). I'll update the pr and remove the dependency on the postgres gem

Why did you add a dependency on addressable that wasn't there before?

It's a runtime dependency for Capybara (added by the capybara gem)

Why did you remove the limits on the text columns (and what does this mean for people submitting maliciously large data)?
Why did you increase the limit on the vote column in the vote table?

This I got to look into as I only added the migration to create one new table (blocked users) and I am unclear as to why this would cause the limit constraints to be removed.

@crertel

This comment has been minimized.

Show comment
Hide comment
@crertel

crertel Nov 28, 2016

Contributor

Thank you for answering my questions. :)

Contributor

crertel commented Nov 28, 2016

Thank you for answering my questions. :)

@pushcx

This comment has been minimized.

Show comment
Hide comment
@pushcx

pushcx Nov 28, 2016

Member

Why did you increase the limit on the vote column in the vote table?

Rails creates schema.rb by introspecting on the database. If PostgresSQL and MySQL differ in their understanding of a migration or implementation of a table, you'll get churn in the file.

The best way to avoid it is to git add -p after running a migration. To fix it, rollback the migration, git add db/schema.rb, re-run the migration, then git add -p only the deliberate changes (which may require manual editing). Then commit and git checkout db/schema.rb to toss the spurious changes.

Member

pushcx commented Nov 28, 2016

Why did you increase the limit on the vote column in the vote table?

Rails creates schema.rb by introspecting on the database. If PostgresSQL and MySQL differ in their understanding of a migration or implementation of a table, you'll get churn in the file.

The best way to avoid it is to git add -p after running a migration. To fix it, rollback the migration, git add db/schema.rb, re-run the migration, then git add -p only the deliberate changes (which may require manual editing). Then commit and git checkout db/schema.rb to toss the spurious changes.

@blueyedgeek

This comment has been minimized.

Show comment
Hide comment
@blueyedgeek

blueyedgeek Dec 2, 2016

@crertel, @pushcx, I've updated the PR (removed the pg gem and retain the limits enforced on particular columns)

blueyedgeek commented Dec 2, 2016

@crertel, @pushcx, I've updated the PR (removed the pg gem and retain the limits enforced on particular columns)

@danielcompton

This comment has been minimized.

Show comment
Hide comment
@danielcompton

danielcompton Mar 23, 2017

After squashing these commits, the rebase to resolve conflicts was simple. What's needed next to move this PR forward?

danielcompton commented Mar 23, 2017

After squashing these commits, the rebase to resolve conflicts was simple. What's needed next to move this PR forward?

@jcs

This comment has been minimized.

Show comment
Hide comment
@jcs

jcs Mar 23, 2017

Contributor

I would like some community discussion on Lobsters on whether this type of feature is wanted.

Contributor

jcs commented Mar 23, 2017

I would like some community discussion on Lobsters on whether this type of feature is wanted.

@danielcompton

This comment has been minimized.

Show comment
Hide comment

danielcompton commented Mar 23, 2017

I've made a meta post about this here: https://lobste.rs/s/debnsq/hiding_comments_by_specific_users

@jcs

This comment has been minimized.

Show comment
Hide comment
@jcs

jcs Mar 28, 2017

Contributor

Sorry, but there didn't seem to be a lot of concensus around wanting this.

Contributor

jcs commented Mar 28, 2017

Sorry, but there didn't seem to be a lot of concensus around wanting this.

@jcs jcs closed this Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment