search

FPGA-Research-Manchester / FABulous

Fabric generator and CAD tools
https://fabulous.readthedocs.io/en/latest/
Apache License 2.0
147 stars 33 forks source link

Add formatter #163

Closed KelvinChung2000 closed 5 months ago

KelvinChung2000 commented 7 months ago

I have added the black workflow. Please let me know if anything else needs to be done besides this yml file.

mole99 commented 7 months ago

Looks good! The CI worklow failed due to invalid formatting.

Now developers need to be instructed to use black before submitting their changes. One way to do this is to add a badge in the README, as OpenLane 2 does for example: https://github.com/efabless/openlane2 But I think it would also be good to mention that in the documentation. And last but not least, this PR should probably change the formatting to be compatible with black.

mole99 commented 7 months ago

Once this PR is merged into the master branch (I think it makes sense to merge it to master since it does not change any functionality) FABulous2.0-development needs to be rebased. If my PR #154 proves difficult for rebasing, you can simply delete and recreate FABulous2.0-development and I will open my PR on top of it again.

KelvinChung2000 commented 7 months ago

Will need to write a section on contribution guidelines.

KelvinChung2000 commented 7 months ago

I also need to avoid significant code base changes. Even though this is only changing the look, who knows, I have broken something mid-way when the CI is not exhaustive enough. I will put this into 2.0 for now so the setup tool can proceed.

mole99 commented 7 months ago

No need to. Black checks the AST to make sure nothing has changed:

Also, as a safety measure which slows down processing, Black will check that the reformatted code still produces a valid AST that is effectively equivalent to the original (see the Pragmatism section for details). If you're feeling confident, use --fast.

I would prefer this to be merged into master.

mole99 commented 6 months ago

Hi! I just wanted to ask if this is ready to merge? It's easier to develop on top of these changes as they touch the entire codebase rather than fixing conflicts later on.

I would suggest merging #164 first and then this PR to master since they are fixes/formatting changes. Then #110 can be rebased and also merged to master, so that a package of current FABulous can be uploaded to PyPI. Then a new 2.0 development branch can be created where I can reapply #154 onto. Further changes should then be done on the 2.0 branch only to avoid conflicts. You could also think about just calling the branch dev like other projects do.

Thanks!

KelvinChung2000 commented 6 months ago

Dirk would like to keep this merge happen later. How about you keep the development going with the formatting applied locally? When it's ready, I will merge this first and follow up with your updates. The amount of conflict should be minimal in that case.

mole99 commented 6 months ago

Okay, will do! Right now I am not actively working on FABulous, but will get back to it later. I just thought it would be nice to finally have a package on PyPI and #110 is waiting for this PR.

mole99 commented 5 months ago

@KelvinChung2000 is there any news on this?

Would it be possible to merge this PR now and apply the formatter on the feature being worked on in the background? In that case, the conflicts should also be minimal. Then #110 could rebase on this PR and we could finally have a Python package of the current FABulous.

KelvinChung2000 commented 5 months ago

The workshop is over. I will ask Dirk what he thinks. I will at least merge this to development first so everyone has a common ground.

mole99 commented 5 months ago

Thanks! :+1: