Skip to content
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

Add vim modeline. #420

Merged
merged 1 commit into from
Sep 2, 2024
Merged

Add vim modeline. #420

merged 1 commit into from
Sep 2, 2024

Conversation

dragonwu0919
Copy link
Contributor

I add vim modeline

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

@dragonwu0919
Copy link
Contributor Author

By the way, I have found many modeline written in different form. Should these modeline be re-written?

for example, instead of // vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:, some the other forms could be:

/* vim: set et ts=4 sw=4: */

// vim: set ff=unix fenc=utf8 nobomb et sw=4 ts=4 sts=4:

@tigercosmos
Copy link
Collaborator

I think we can use the longest version as it includes all other versions' settings. 🤔

@dragonwu0919
Copy link
Contributor Author

@yungyuc I have added one vim modeline. Please help me review it. Thank.

@ThreeMonth03
Copy link
Collaborator

ThreeMonth03 commented Aug 28, 2024

In modmesh/gtests/, it seems that the vim modeline should be added in some of the files. @dragonwu0919, could you please check more files and add more missing vim modelines? Thank you.

@yungyuc
Copy link
Member

yungyuc commented Aug 29, 2024

In modmesh/gtests/, it seems that the vim modeline should be added in some of the files. @dragonwu0919, could you please check more files and add more missing vim modelines? Thank you.

I concur. @dragonwu0919 it will help you get familiar with the code base to scan through all files for a specific style conformity/violation. Why don't you compile a list of all files that need to fix for the modeline?

@dragonwu0919
Copy link
Contributor Author

dragonwu0919 commented Aug 29, 2024

@ThreeMonth03 Sure, I can help add the missing vim modeline back.

@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

@ThreeMonth03
Copy link
Collaborator

ThreeMonth03 commented Aug 29, 2024

@ThreeMonth03 Sure, I can help add the missing vim modeline back.

@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

It looks like you've successfully rebased and squashed the commits. @dragonwu0919, could you please remove the "Add Vim modelines 2" commit message because it is meaningless? Thank you.

@dragonwu0919
Copy link
Contributor Author

dragonwu0919 commented Aug 29, 2024

@ThreeMonth03 Sure, I can help add the missing vim modeline back.
@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

It looks like you've successfully rebased and squashed the commits. @dragonwu0919, could you please remove the "Add Vim modelines 2" commit because it is meaningless? Thank you.

@ThreeMonth03 Sorry for asking a basic question, but may i ask how to remove that commit? My log shows that the commit "Add Vim modeline 2" has been deleted in my local repo.

PS C:\SciWork\modmesh> git log --oneline
cbfa15a (HEAD -> CodeStyle) Add vim modeline.
f7195c7 (origin/master, origin/HEAD, master) Merge pull request #418 from Tucchhaa/expose_camera_api_python
42161b5 apply review 2

@ThreeMonth03
Copy link
Collaborator

@ThreeMonth03 Sure, I can help add the missing vim modeline back.
@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

It looks like you've successfully rebased and squashed the commits. @dragonwu0919, could you please remove the "Add Vim modelines 2" commit because it is meaningless? Thank you.

@ThreeMonth03 Sorry for asking a basic question, but may i ask how to remove that commit? My powershell says that the commit "Add Vim modeline 2" has been deleted in my local repo.

PS C:\SciWork\modmesh> git log --oneline
cbfa15a (HEAD -> CodeStyle) Add vim modeline.
f7195c7 (origin/master, origin/HEAD, master) Merge pull request #418 from Tucchhaa/expose_camera_api_python
42161b5 apply review 2
``

Sorry for my unclear explanation. I think you should rebase and reword the commit cbfa15a, and you could refer the note to do it.

@ThreeMonth03
Copy link
Collaborator

@ThreeMonth03 Sure, I can help add the missing vim modeline back.
@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

It looks like you've successfully rebased and squashed the commits. @dragonwu0919, could you please remove the "Add Vim modelines 2" commit because it is meaningless? Thank you.

@ThreeMonth03 Sorry for asking a basic question, but may i ask how to remove that commit? My log shows that the commit "Add Vim modeline 2" has been deleted in my local repo.

PS C:\SciWork\modmesh> git log --oneline
cbfa15a (HEAD -> CodeStyle) Add vim modeline.
f7195c7 (origin/master, origin/HEAD, master) Merge pull request #418 from Tucchhaa/expose_camera_api_python
42161b5 apply review 2

BTW, if you enter git log in your terminal, you should see that the second line of the commit cbfa15a is Add Vim modeline 2, and you could reword the commit cbfa15a and remove this line in the commit message.

@ThreeMonth03
Copy link
Collaborator

Thanks, @dragonwu0919, the commit message is satisfying.

@dragonwu0919
Copy link
Contributor Author

dragonwu0919 commented Aug 29, 2024

@ThreeMonth03 Sure, I can help add the missing vim modeline back.
@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

It looks like you've successfully rebased and squashed the commits. @dragonwu0919, could you please remove the "Add Vim modelines 2" commit because it is meaningless? Thank you.

@ThreeMonth03 Sorry for asking a basic question, but may i ask how to remove that commit? My log shows that the commit "Add Vim modeline 2" has been deleted in my local repo.

PS C:\SciWork\modmesh> git log --oneline
cbfa15a (HEAD -> CodeStyle) Add vim modeline.
f7195c7 (origin/master, origin/HEAD, master) Merge pull request #418 from Tucchhaa/expose_camera_api_python
42161b5 apply review 2

BTW, if you enter git log in your terminal, you should see that the second line of the commit cbfa15a is Add Vim modeline 2, and you could reword the commit cbfa15a and remove this line in the commit message.

@ThreeMonth03 Thank you for the reminding. I never thought that --oneline will function like this. I misunderstood this command into squish commit log into one line, instead of show only the first line of commit log .

@dragonwu0919
Copy link
Contributor Author

@ThreeMonth03 Sure, I can help add the missing vim modeline back.

@yungyuc Thank you for the review and the suggestion. I will try to compile them when fixing modelines.

@yungyuc Sorry for bothering you further. You suggested that I could compile those files when fixing the modeline problem, but I don't understand what the word compile means in this context. Should I literally find a compiler to compile the files, or is it just suggesting that I could read the pattern of how the code is written here?

@yungyuc
Copy link
Member

yungyuc commented Aug 30, 2024

@yungyuc Sorry for bothering you further. You suggested that I could compile those files when fixing the modeline problem, but I don't understand what the word compile means in this context. Should I literally find a compiler to compile the files, or is it just suggesting that I could read the pattern of how the code is written here?

By "compile a list of all files that need to fix for the modeline", I meant to make a list of all files that need to fix for the modeline.

@dragonwu0919
Copy link
Contributor Author

@yungyuc Sorry for bothering you further. You suggested that I could compile those files when fixing the modeline problem, but I don't understand what the word compile means in this context. Should I literally find a compiler to compile the files, or is it just suggesting that I could read the pattern of how the code is written here?

By "compile a list of all files that need to fix for the modeline", I meant to make a list of all files that need to fix for the modeline.

Understood. I will try my best to do so.

@dragonwu0919 dragonwu0919 force-pushed the CodeStyle branch 3 times, most recently from eea8f36 to eb80816 Compare August 31, 2024 09:35
@dragonwu0919
Copy link
Contributor Author

@yungyuc The files that have been updated with Vim modelines are listed below. Please review the overall changes. Thank you.

{
    "WithoutModeline":  [
                       ".\\setup.py",
                       ".\\cpp\\modmesh\\buffer\\pymod\\TypeBroadcast.hpp",
                       ".\\cpp\\modmesh\\inout\\pymod\\wrap_Gmsh.cpp",
                       ".\\cpp\\modmesh\\inout\\pymod\\wrap_Plot3d.cpp",
                       ".\\cpp\\modmesh\\onedim\\Euler1DCore.cpp",
                       ".\\cpp\\modmesh\\onedim\\Euler1DCore.hpp",
                       ".\\cpp\\modmesh\\spacetime\\kernel\\BadEuler1DSolver.cpp",
                       ".\\cpp\\modmesh\\spacetime\\kernel\\BadEuler1DSolver.hpp",
                       ".\\cpp\\modmesh\\testhelper\\pymod\\testbuffer_pymod.cpp",
                       ".\\modmesh\\params.py",
                       ".\\modmesh\\app\\euler1d.py",
                       ".\\tests\\test_gmsh.py"
                   ]
}

@yungyuc yungyuc added the refactor Change code without changing tests label Aug 31, 2024
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

The files that have been updated with Vim modelines are listed below. Please review the overall changes. Thank you.

{
    "WithoutModeline":  [
                       ".\\setup.py",
                       ".\\cpp\\modmesh\\buffer\\pymod\\TypeBroadcast.hpp",
                       ".\\cpp\\modmesh\\inout\\pymod\\wrap_Gmsh.cpp",
                       ".\\cpp\\modmesh\\inout\\pymod\\wrap_Plot3d.cpp",
                       ".\\cpp\\modmesh\\onedim\\Euler1DCore.cpp",
                       ".\\cpp\\modmesh\\onedim\\Euler1DCore.hpp",
                       ".\\cpp\\modmesh\\spacetime\\kernel\\BadEuler1DSolver.cpp",
                       ".\\cpp\\modmesh\\spacetime\\kernel\\BadEuler1DSolver.hpp",
                       ".\\cpp\\modmesh\\testhelper\\pymod\\testbuffer_pymod.cpp",
                       ".\\modmesh\\params.py",
                       ".\\modmesh\\app\\euler1d.py",
                       ".\\tests\\test_gmsh.py"
                   ]
}

Thanks. It is clear. Having said that, it will look nicer in the review by using a markdown list:

  • setup.py
  • cpp/modmesh/buffer/pymod/TypeBroadcast.hpp
  • cpp/modmesh/inout/pymod/wrap_Gmsh.cpp
  • cpp/modmesh/inout/pymod/wrap_Plot3d.cpp
  • cpp/modmesh/onedim/Euler1DCore.cpp
  • cpp/modmesh/onedim/Euler1DCore.hpp
  • cpp/modmesh/spacetime/kernel/BadEuler1DSolver.cpp
  • cpp/modmesh/spacetime/kernel/BadEuler1DSolver.hpp
  • cpp/modmesh/testhelper/pymod/testbuffer_pymod.cpp
  • modmesh/params.py
  • modmesh/app/euler1d.py
  • tests/test_gmsh.py

(Simple regular expressions can convert the json to the markdown list.)


Need to change:

  • Make sure all line endings uses Unix LF (\n).

@@ -234,3 +234,5 @@ struct TypeBroadcast

} /* end namespace python */
} /* end namespace modmesh */
\r\n
Copy link
Member

Choose a reason for hiding this comment

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

It seems that you have some lines mixed with dos/windows CF+LF plain-text line ending. Please correct them.

You probably did not configure your editor correctly.

Copy link
Collaborator

@ThreeMonth03 ThreeMonth03 left a comment

Choose a reason for hiding this comment

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

Comment on lines 302 to 304
} /* end namespace modmesh */ No newline at end of file
} /* end namespace modmesh */
// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

@ThreeMonth03 ThreeMonth03 Aug 31, 2024

Choose a reason for hiding this comment

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

I'm not sure whether there should be a newline between the bracket and the vim modeline. @yungyuc, how do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It is better to leave a blank line between the modeline and the code above it.

} // namespace modmesh

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline (\n).

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

It seems that you miss to add a newline after the vim modeline.

Comment on lines 842 to 844

# vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

In the editor, it is clear that there are two newlines after the vim modeline, and the github action should fail under this circumstance.
Thus, you may modify this part according to the following rules:

  1. There should be only a newline after the vim modeline.
  2. The newline should not contain any character, such as the tab '\t' and the space .

Comment on lines 238 to 239
// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 newlines.

Comment on lines 48 to 49
// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 newlines.

} // namespace modmesh

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing newline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that you miss to add a newline after the vim modeline.

image

Comment on lines 113 to 114
# vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 newlines.

setup.py Outdated
Comment on lines 83 to 84
# vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 newlines.

Comment on lines 37 to 38
# vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 newlines.

@@ -515,3 +515,5 @@ SimpleArrayPlex::~SimpleArrayPlex()
}

} /* end namespace modmesh */

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is only one newline after the vim modeline, good job.

@dragonwu0919 dragonwu0919 force-pushed the CodeStyle branch 2 times, most recently from c0c7428 to 3b58c72 Compare September 1, 2024 05:44
@dragonwu0919
Copy link
Contributor Author

  • Ensured there is only one newline after the modeline.
  • Ensured there is a newline between the Vim modeline and the rest of the file content.

Hope it doesn't go wrong this time. @yungyuc, could you please help me review the changes? Thank you!

Copy link
Collaborator

@ThreeMonth03 ThreeMonth03 left a comment

Choose a reason for hiding this comment

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

@dragonwu0919 I think after the slight modification to the above files, we could merge this pr. Thank's for your contribution.

} // namespace modmesh

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

It seems that you miss to add a newline after the vim modeline.

@@ -314,3 +314,5 @@ TEST(Gmsh_Parser, Hexahedron125NodeDefinition)
EXPECT_EQ(ele_def.mmtpn(), 5);
EXPECT_THAT(ele_def.mmcl(), testing::ElementsAre(0, 1, 2, 3, 4, 5, 6, 7));
}

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that you miss to add a newline after the vim modeline.

image

} // namespace modmesh

// vim: set ff=unix fenc=utf8 et sw=4 ts=4 sts=4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that you miss to add a newline after the vim modeline.

image

@yungyuc
Copy link
Member

yungyuc commented Sep 1, 2024

@dragonwu0919 I think after the slight modification to the above files, we could merge this pr. Thank's for your contribution.

Agree.


It would be nice to have consistent new line after the modeline, but this is indeed very easy to miss when we only review a limited number of source files.

When it is for developing a feature or fixing a bug, the inconsistency at the end of file is fine. Picking a minor issue like that after a great effort of hacking can be discouraging.

But for a (style) refactoring PR like this, it good to make the style as consistent as possible, because its sole purpose is to make the style perfect.

@dragonwu0919
Copy link
Contributor Author

I have completed the modifications for these three files. @yungyuc, @ThreeMonth03, thank you both very much. This experience has really helped me become more familiar with how PRs work.

Copy link
Collaborator

@ThreeMonth03 ThreeMonth03 left a comment

Choose a reason for hiding this comment

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

@dragonwu0919 Thanks for your contribution. Everything looks fine now.

@ThreeMonth03
Copy link
Collaborator

ThreeMonth03 commented Sep 1, 2024

@dragonwu0919 Thanks for your contribution. Everything looks fine now.

@dragonwu0919 We are sorry to inform you that you haven't pass all of the github workflows, and the detail of the error log could find here. You must to pass all of the the github action before merging the pr.
image

BTW, if you want to run the github action under your repository, you can refer this article, then your repository will run the github workflows automatically.

The C++ and Python files in this project have been scanned. Modelines have been added to those files that had no modeline at all. For files containing a modeline other than the latest version, I left them unchanged.
@ThreeMonth03
Copy link
Collaborator

@dragonwu0919 Thanks for your contribution. Everything looks fine now.

@dragonwu0919 We are sorry to inform you that you haven't pass all of the github workflows, and the detail of the error log could find here. You must to pass all of the the github action before merging the pr. image

BTW, if you want to run the github action under your repository, you can refer this article, then your repository will run the github workflows automatically.

@dragonwu0919, thank you very much. It seems that you correct the errors and pass the workflows, and it looks good to me. @yungyuc, you could review this pr when you are available, thank you.

@yungyuc yungyuc merged commit da4328f into solvcon:master Sep 2, 2024
12 checks passed
@yungyuc
Copy link
Member

yungyuc commented Sep 2, 2024

Thanks, @dragonwu0919 . Look good to me. Merging.

@dragonwu0919 dragonwu0919 deleted the CodeStyle branch September 10, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Change code without changing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants