Ifcopenshell: BlenderBIM Python coding style

[htlcnn]

@Moult
Could we agree on some coding style to apply to BlenderBIM Python code? We could use PEP8 with some exceptions of 80-char-line-length.
I usually use VS Code with black for my code formatter and flake8 for my linter.
After some agreements, I might go cleaning up the code.
Thanks

[Moult]

I'm happy to use black and flake8. A quick google shows both as solid recommendations.

Before proceeding, we'll need to ping @aothms for his opinion, as I suspect this decision would also impact things like IfcOpenShell Python modules.

[Moult]

Ping @aothms ?

[htlcnn]

He might be enjoying the weekend 😃

[Moult]

Now I feel guilty for pinging! :) Absolutely no rush at all...

[aothms]

No worries. I had a message written somewhere but apparently forgot to
send. Either is fine to me. Would be good if we document it on the wiki
(there is a style page about c++) and test on Travis.

Sent from a mobile device. Excuse my brevity. Kind regards, Thomas

Op zo 1 nov. 2020 08:53 schreef Dion Moult [e-mail]:

Now I feel guilty for pinging! :) Absolutely no rush at all...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/IfcOpenShell/IfcOpenShell/issues/1057#issuecomment-720050121,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAILWV3C4HGCUH6GJW3ASNLSNUHW5ANCNFSM4TDM3KPQ
.

[Moult]

Cheers :) Go go go @htlcnn ! I'll update the wiki once the code cleanup is in progress!

[htlcnn]

@Moult what about line length limit? PEP8 suggests 80, I use 120.

[Moult]

120 sounds more sensible with today's monitors and IDEs. Also, I tend to like long, descriptive variable names.

[htlcnn]

@Moult
I used black to reformat src folder. Please have a look: https://github.com/IfcOpenShell/IfcOpenShell/pull/1067
naming would be consider later as it takes time. I agree that explicit is better than implicit :)

[Moult]

I've closed the PR, not because I disagree, but because it is quite a long PR. 114 files is a lot to review, and I suspect I'll be making other changes in the meantime, and I want to avoid merge conflicts.

Can you create a new PR that only reformats files in these dirs:

• src/bimtester/
• src/ifcclash/
• src/ifccobie/
• src/ifccsv/
• src/ifcdiff/
• src/ifcpatch/
• src/ifcsverchok/

That will be much faster to review, and then once I approve the PR, you can create another PR for src/ifcblenderexport/ and src/ifcopenshell-python.

Then, ping @Jesusbill to see if he is OK with standardising the coding style, and then you can run it on src/ifc2ca.

[Jesusbill]

Yes it is fine for me

[htlcnn]

@Moult sure, here you are: https://github.com/IfcOpenShell/IfcOpenShell/pull/1068

[Moult]

Merged :) Now for the Blender add-on and IfcOpenShell Python :)

[htlcnn]

@Moult https://github.com/IfcOpenShell/IfcOpenShell/pull/1069 thanks :)

[CyrilWaechter]

@htlcnn I like black and think it is a good thing to use it. However it would have been nice to use technique which allow to not ruin git blame : Migrating your code style without ruining git blame

[Moult]

@CyrilWaechter that's a good idea! @htlcnn would you be interested in committing a git blame ignore file?

[htlcnn]

@CyrilWaechter thanks for the suggestion.
For the convenience of reviewing the PR, I used 1 commit for each time of a folder reformatting as @Moult asked. I found it reasonable. If you want to ignore those commits when your editor runs git blame, you have to configure custom argument --ignore-revs-file .git-blame-ignore-revs. I did that with my editor (VS Code), it worked for the comment on current line, but not on the status bar (please refer to the attached img).

And please note that

GitHub and GitLab do not yet support ignoring revisions using their native UI of blame

If you don't mind setting up your editor to add custom argument to git blame, I'll push a .git-blame-ignore-revs file. If that's the case, I think this should be documented in the wiki for other devs to setup their editors.

[htlcnn]

@Moult ping

[Moult]

Happy to get a git blame ignore file, it doesn't hurt, and we can always add more to it in the future if we need more commits to be ignored.

[htlcnn]

@Moult https://github.com/IfcOpenShell/IfcOpenShell/pull/1073

[Moult]

Thanks so much! Closing bug, as I think this is now resolved fully!

[CyrilWaechter]

@Moult Almost fully resolved. aothms asked to add it to wiki. I assume he was talking about github wiki so I have added a python paragraph. See if it is ok for you.

He also asked something about test on Travis but I did not fully understood.

[htlcnn]

@CyrilWaechter we might add line length limit of 120 characters to the wiki as discussed above.
Travis is to automatically test whether a commit complies to the styling we chose. Some commands should be added to https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.6.0/.travis.yml but I see only python2.7 there. Could you please elaborate @aothms ?

[CyrilWaechter]

@CyrilWaechter we might add line length limit of 120 characters to the wiki as discussed above.

Done also added pyproject.toml to avoid the need of manual configuration.

[aothms]

Does it help if I update Travis yml to py3?

Sent from a mobile device. Excuse my brevity. Kind regards, Thomas

Op wo 4 nov. 2020 09:00 schreef Dion Moult [e-mail]:

Closed #1057 https://github.com/IfcOpenShell/IfcOpenShell/issues/1057.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/IfcOpenShell/IfcOpenShell/issues/1057#event-3956286453,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAILWV7VJXU2JY27GCJFHPLSOECYXANCNFSM4TDM3KPQ
.

[htlcnn]

@aothms Because I don't see how travis runs on this repo, may I ask what are you using .travis.yml for? Is it for IfcOpenBot to test, then build if the test succeeds? Why does IfcOpenShell still support Python 2.7?
black requires Python 3.6+ to work, so we should focus on using flake8 to do linting in the automatic workflow. What are your thoughts?

[aothms]

Like I said, I don't mind upgrading to python3.6. I'll add it to my list.

Travis CI is running, but I think there is a bug in their integration somehow as they no longer report back to the Github API, but the builds are running: https://travis-ci.org/github/IfcOpenShell/IfcOpenShell/builds

[htlcnn]

@aothms flake8 can run on Python2.7, I was too curious.
I made a PR to add flake8 to travis-ci. Please have a look: https://github.com/IfcOpenShell/IfcOpenShell/pull/1080
Thanks.
P/S: the purpose of adding flake8 to travis is to test whether python files in a commit comply to PEP8. If you want to automatically format the files, travis must run Python 3.6+, but I don't feel it's necessary because we have wiki to guide devs to setup their environments.

[htlcnn]

https://travis-ci.org/github/IfcOpenShell/IfcOpenShell/builds/741738797
@aothms @Moult flake8 ran and made the build fail. Should we check all .py files or just specific folders such as blenderbim?

[htlcnn]

I updated travis-ci badge here: https://github.com/IfcOpenShell/IfcOpenShell/pull/1085

[htlcnn]

I think this can be closed now. IMO coding style should be a recommendation, not mandatory. It's now in the wiki.