Skip to content

Conversation

pjhoberman
Copy link
Collaborator

See notes in #17. This is in draft for now.

@quinn-dougherty
Copy link
Collaborator

@pjhoberman i'm gonna do the auto-update from here sooner rather than later, fetch it down please hope it doesn't cause conflict

@quinn-dougherty
Copy link
Collaborator

nvm it said there's a merge conflict already

@pjhoberman
Copy link
Collaborator Author

@quinn-dougherty rebased and re-pushed. It's in draft mode while I wait for feedback though

@sam-writer
Copy link
Collaborator

Just to check - there is a big formatting change as well, that is intentional? I see @mdbecker 's commit running black (is that part of CI/CD?) as part of this PR... just trying to understand that.

About the real content of this PR - I like the way it is done. I have one code style thought, but nothing blocking. It seems like the only purpose of the second param, def chart_descriptions(chart, census=False) is

    if census:
        cols = [col + " Census" for col in cols]

it might be more extensible to just make the second parameter the word we want to add, eg def chart_descriptions(chart, suffix="") and do something like if suffix != "": cols = [col + " " + suffix for col in cols]

But like I said, not blocking

@quinn-dougherty
Copy link
Collaborator

black is not in ci/cd yet #193

@pjhoberman
Copy link
Collaborator Author

@sam-qordoba The formatting updates are from the rebase. I'm not sure how to avoid pulling those commits in when I rebase - any thoughts?

I like the suffix idea. It's a bit fragile in that it's also related to the column name in the df. Trying to balance DRY. I can update that though!

@pjhoberman pjhoberman marked this pull request as ready for review March 23, 2020 18:13
@pjhoberman pjhoberman requested a review from sam-writer March 23, 2020 18:13
@sam-writer
Copy link
Collaborator

@pjhoberman yeah I mean, it is still brittle so it might not matter. Just a thought.

@quinn-dougherty or @pjhoberman or @mdbecker - is this the way that @mdbecker 's formatting changes are coming into the dev branch? or are his changes in another branch also? I am fine with either, just curious

@pjhoberman pjhoberman linked an issue Mar 23, 2020 that may be closed by this pull request
Copy link
Collaborator

@sam-writer sam-writer left a comment

Choose a reason for hiding this comment

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

LGTM

@mdbecker mdbecker merged commit 4177e46 into develop Mar 23, 2020
@mdbecker mdbecker deleted the 17-chart-language branch March 23, 2020 19:30
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.

Natural language summary of outputs
4 participants