Skip to content

Conversation

@dstandish
Copy link
Contributor

Not used.

@jedcunningham
Copy link
Member

We should consider es_read as private, yeah?

@dstandish dstandish force-pushed the remove-unused-arg-metadata branch from 322cd26 to 6ddeaa3 Compare October 5, 2023 22:26
@dstandish
Copy link
Contributor Author

We should consider es_read as private, yeah?

Oh boy...
well... maybe technically we can't but 🤷

@dstandish
Copy link
Contributor Author

airflow should make things all private unless a @public decorator is added.... default private everywhere

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

My take is it's internal to the ES log handler, shouldn't be a problem to remove it.

@dstandish
Copy link
Contributor Author

My take is it's internal to the ES log handler, shouldn't be a problem to remove it.

well, if we're gonna do that, should we go all out and make it _es_read / :meta private:?

@eladkal
Copy link
Contributor

eladkal commented Oct 6, 2023

My take is it's internal to the ES log handler, shouldn't be a problem to remove it.

I'm fine with it but lets at least add entry about this in the change log that explains this decision. Simply log it at the top of the provider change log I will handle the style and place it in the correct place during release.

@jedcunningham
Copy link
Member

well, if we're gonna do that, should we go all out and make it _es_read / :meta private:?

Works for me.

@dstandish
Copy link
Contributor Author

I'm fine with it but lets at least add entry about this in the change log that explains this decision. Simply log it at the top of the provider change log I will handle the style and place it in the correct place during release.

OK thanks Elad

@dstandish dstandish changed the title Remove unused argument metadata from es_read Remove unused argument metadata from es_read and make clearly private Oct 6, 2023
@dstandish
Copy link
Contributor Author

ok @eladkal i added the docs note. i also renamed it _es_read. if you think the method rename goes too far LMK

Co-authored-by: Elad Kalif <45845474+eladkal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants