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

Annoy black #293

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Annoy black #293

merged 1 commit into from
Oct 20, 2022

Conversation

redphix
Copy link
Contributor

@redphix redphix commented Oct 20, 2022

What does this PR do?

annoy black

Checklist before merging

  • If it's a frontend feature, I have ran prettier cd frontend; npm run format. If it's a mobile app feature I ran cd mobile; npm run format.

@redphix
Copy link
Contributor Author

redphix commented Oct 20, 2022

Okay nice. The flake8 lints work with annotations.

@redphix
Copy link
Contributor Author

redphix commented Oct 20, 2022

@Reckless-Satoshi
But the black auto-fix should only work when you either

  1. Open a PR yourself
  2. push to main

looks fine for now. What do you think?

The reason you don't see black annotating the commits is that they get fixed due to auto_fix so the action assumes that you don't want to annotate the commits and just fix them. Looks alright to me. The annotations that black displays aren't really lint errors, but diffs -- it tells you what to delete/add to conform to black style guide, which I don't find that user friendly. It's a diff, which indicates just let it do its thing and auto apply it.

@redphix redphix marked this pull request as ready for review October 20, 2022 14:07
@Reckless-Satoshi
Copy link
Collaborator

looks fine for now. What do you think?

I think it's a way better set up than what we had before! 😄 Thank you very much for getting this done 🚀

@redphix
Copy link
Contributor Author

redphix commented Oct 20, 2022

Final test I guess -- merging this should run the auto-fix and commit it to main branch.

@Reckless-Satoshi Reckless-Satoshi merged commit aa44541 into RoboSats:main Oct 20, 2022
@Reckless-Satoshi
Copy link
Collaborator

Final test I guess -- merging this should run the auto-fix and commit it to main branch.

Merged. Auto-fix did not add a new commit, possibly because it also needs on.pull_request event as trigger. In any case, do we want auto fix commits? It is probably enough with PR checks. Also a bit scary the bot committing changes to main :D

@redphix
Copy link
Contributor Author

redphix commented Oct 21, 2022

@Reckless-Satoshi Yes, I was also having second thoughts about it :D. A bot commit changes to main is definitely an uncomfortable feeling. I guess we don't autofix then -- commit annotations are more than enough. The auto-fixing part can be documented/enforced using pre-commit, but again, it just formatting, so don't think it's that serious - on the fence with this..you decide xD

@redphix redphix deleted the test-lint-workflow branch October 21, 2022 05:41
@redphix
Copy link
Contributor Author

redphix commented Oct 21, 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.

2 participants