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 formatter #163

Merged
merged 7 commits into from
Apr 25, 2024
Merged

Add formatter #163

merged 7 commits into from
Apr 25, 2024

Conversation

KelvinChung2000
Copy link
Collaborator

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

@mole99
Copy link
Contributor

mole99 commented Feb 26, 2024

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
Copy link
Contributor

mole99 commented Feb 26, 2024

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
Copy link
Collaborator Author

Will need to write a section on contribution guidelines.

@KelvinChung2000 KelvinChung2000 changed the base branch from master to FABulous2.0-development February 27, 2024 13:31
@KelvinChung2000
Copy link
Collaborator Author

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
Copy link
Contributor

mole99 commented Feb 27, 2024

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
Copy link
Contributor

mole99 commented Mar 11, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

mole99 commented Mar 11, 2024

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
Copy link
Contributor

mole99 commented Apr 25, 2024

@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
Copy link
Collaborator Author

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.

@KelvinChung2000 KelvinChung2000 merged commit 523d7e3 into FPGA-Research:FABulous2.0-development Apr 25, 2024
1 check passed
@mole99
Copy link
Contributor

mole99 commented Apr 27, 2024

Thanks! 👍

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.

2 participants