Skip to content

Conversation

@billishyahao
Copy link
Contributor

The end of line in test_dnnl.py is CRLF which is not identical to other python files. This patch is going to fix this by replacing CRLF to LF.

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

@billishyahao
Copy link
Contributor Author

billishyahao commented Jun 3, 2022

Hi @masahi, @AndrewZhaoLuo , Could you please review this change?

@masahi
Copy link
Member

masahi commented Jun 3, 2022

Why do we need to fix this?

@AndrewZhaoLuo
Copy link
Contributor

If I understand this change correctly this is entirely style; git will handle different line deliminators fine, but the issue is that if a windows dev edits a file with LF, it will replace all lines with CRLF through the entire file which means every line will be changed in diffs. This will make the commit log a bit gummier.

cc @areusch @driazati it seems like a good idea to force LF for new lines? can we lint for this.

@driazati
Copy link
Member

driazati commented Jun 7, 2022

We should definitely check for this kind of thing in CI, opened #11609

@billishyahao
Copy link
Contributor Author

Hi @masahi , @AndrewZhaoLuo , I hit this issue when I am trying to accomplish my another PR #11513 . It seems that somebody use windows as development environment and unexpectedly reformat the whole LF to CRLF. Git status command shows this change and it is annoying for other developer of tvm. To eliminate this issue, I propose this patch.
As Andrew 's words, it is better to enable lint to avoid other people to change LF to CRLF unexpectedly.
@AndrewZhaoLuo Hi, may I continue merging this change?

@masahi masahi merged commit a95a820 into apache:main Jun 8, 2022
Kathryn-cat pushed a commit to Kathryn-cat/tvm that referenced this pull request Jun 10, 2022
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.

4 participants