Skip to content

Conversation

@feluelle
Copy link
Member

@feluelle feluelle commented Feb 3, 2020

Note: From Python 3.x onwards the explicit utf-8 header is no longer required. It is utf-8 encoded by default.


Issue link: Document only change, no JIRA issue

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@feluelle
Copy link
Member Author

feluelle commented Feb 3, 2020

Related to #7338

@mik-laj
Copy link
Member

mik-laj commented Feb 3, 2020

I want to do it using pyupgrade: #7343 I don't know if we want to duplicate tools that do the same.

@potiuk
Copy link
Member

potiuk commented Feb 3, 2020

I think we can add it for now and replace with pyupgrade when it's ready. Pyupgrade has more things that it fixes but I see no problem with adding it now especially that it is built-in in one of the repos we have now.

@potiuk
Copy link
Member

potiuk commented Feb 3, 2020

However we should exclude vendor files from this for sure

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

As mentioned - for now it's a good solution especially that encodings were added in few files in the meantime. But _vendor should be excluded - similarly as for the other checks.

@mik-laj
Copy link
Member

mik-laj commented Feb 3, 2020

I don't know if adding magic comments with coding is a problem that is worth solving. It seems to me that if someone adds this comment it does not cause serious problems and we do not need a temporary solution. Instead of developing a temporary solution for a maximum of 2 weeks, you can make other contributions. I am not against this change. If you decide to finish it, I will gladly accept it. I just wanted to show the wider context of my change. Deleting the magic of comments is only an intermediate step, but it is not my goal.

@potiuk
Copy link
Member

potiuk commented Feb 3, 2020

I think discussing it now took more time than implementing it :). I am all for intermediate solutions if they are super-simple to implement.

@feluelle feluelle force-pushed the feature/AIRFLOW-XXXX-pre-commit-check-file-encoding branch from eb79ef1 to f9a0010 Compare February 3, 2020 16:17
@potiuk
Copy link
Member

potiuk commented Feb 3, 2020

I think you need to apply the precommit in the next push @feluelle !

@feluelle
Copy link
Member Author

feluelle commented Feb 4, 2020

True. 🤦‍♂

Note: From Python 3.x onwards the explicit utf-8 header is no longer required. It is utf-8 encoded by default.
@feluelle feluelle force-pushed the feature/AIRFLOW-XXXX-pre-commit-check-file-encoding branch from f9a0010 to d50b5bb Compare February 4, 2020 08:02
@boring-cyborg boring-cyborg bot added provider:Apache provider:amazon AWS/Amazon - related issues labels Feb 4, 2020
@feluelle
Copy link
Member Author

feluelle commented Feb 4, 2020

That should do it! ..now I think we should get this thing quickly merged as soon as it passes to avoid further rebases :P

@codecov-io
Copy link

Codecov Report

Merging #7347 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7347      +/-   ##
==========================================
- Coverage   86.35%   86.06%   -0.29%     
==========================================
  Files         871      871              
  Lines       40627    40627              
==========================================
- Hits        35083    34967     -116     
- Misses       5544     5660     +116
Impacted Files Coverage Δ
.../providers/amazon/aws/operators/cloud_formation.py 100% <ø> (ø) ⬆️
...rflow/providers/mysql/operators/presto_to_mysql.py 100% <ø> (ø) ⬆️
...flow/providers/amazon/aws/hooks/cloud_formation.py 96.77% <ø> (ø) ⬆️
...ow/providers/amazon/aws/sensors/cloud_formation.py 100% <ø> (ø) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0%> (-45.08%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 70.21% <0%> (-23.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f7acb1...d50b5bb. Read the comment docs.

@potiuk potiuk merged commit 94fccca into apache:master Feb 4, 2020
@potiuk
Copy link
Member

potiuk commented Feb 4, 2020

Thanks @feluelle !

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
)

Note: From Python 3.x onwards the explicit utf-8 header is no longer required. It is utf-8 encoded by default.
@feluelle feluelle deleted the feature/AIRFLOW-XXXX-pre-commit-check-file-encoding branch April 30, 2020 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants