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

Fixed PI/task instance caluclation #1089

Merged
merged 2 commits into from
Jul 27, 2022
Merged

Fixed PI/task instance caluclation #1089

merged 2 commits into from
Jul 27, 2022

Conversation

berndruecker
Copy link
Member

No description provided.

@berndruecker berndruecker requested a review from pepopowitz July 27, 2022 07:09
@akeller
Copy link
Member

akeller commented Jul 27, 2022

Looks like prettier isn't running. I assume @berndruecker created this PR from the browser.

@pepopowitz
Copy link
Collaborator

Yeah, it's frustrating to run into this. I'll spend some time investigating how to get the GitHub UI to run hooks.

@berndruecker can you please run npm run format on this branch & commit to fix the formatting?

@berndruecker
Copy link
Member Author

I wonder if it might be easier if some of you quickly run this? For me, it is not daily business to have the environment open and I would expect a higher lead time :-( That said - if this is the best process we want to follow, I can certainly make it work...

@berndruecker
Copy link
Member Author

Ì also wondered if we actually need prettyfier? What's the value of it? I simply changed two values in a table - why is that blocking the build?

@pepopowitz
Copy link
Collaborator

I wonder if it might be easier if some of you quickly run this? For me, it is not daily business to have the environment open and I would expect a higher lead time :-( That said - if this is the best process we want to follow, I can certainly make it work...

I will run this for this PR, though I want to make it clear that I am not interested in a workflow where "Steve will always run prettier for PRs opened through GitHub's UI." We have a contribution guide which states that code/docs must be formatted according to our standards -- I think it's important for all contributors to follow this. My preference is that if you (or any contributor) opens a PR, and the checks fail due to improper formatting, you (or any contributor) corrects the improper formatting, or explicitly requests that someone else correct it for them. This is maybe a minor detail but it's important to me, to avoid acquiring unwritten responsibilities of fixing a subset of PRs.

Longer term -- it is not cool that anyone has to make this formatting commit. Bots should be doing this for us, as they're doing for every commit that isn't made through the GitHub UI. This is an irritating gap in our automation and I'd like to close it.

Action Items from this comment

  • I will correct the formatting on this PR
  • I will open a PR to the CONTRIBUTING.md specifying that requesting that a reviewer correct the formatting is always an option for contributors, if they don't want to configure a local environment.
  • I will add an issue for me to investigate automating the formatting of commits made through GitHub's UI. If we can't make GitHub run the pre-commit hooks, we could add a bot that responds to a comment like "/format", runs prettier, and commits it.

@pepopowitz
Copy link
Collaborator

Ì also wondered if we actually need prettyfier? What's the value of it? I simply changed two values in a table - why is that blocking the build?

The reason we run prettier is to enforce formatting standards, which helps us avoid PRs that contain formatting changes due to (a) differences of opinion on what the formatting should look like, and (b) differences in tooling.

For 99% of PRs, the configured pre-commit hook takes care of it for us by formatting the changes automatically. It's this 1% that happen through GitHub's UI that evade the auto-formatting -- as I state in my previous comment, this is a gap in our automation, and it'll be a pain until I can close it.

More specifically, this PR failed because the formatting of the table doesn't match the rest of our code. The formatting of the table might not matter to you; it doesn't matter to me, to be honest...but the consistency across the codebase is important to me. It reduces time spent trying to figure out how I should format things and it reduces time spent reviewing PRs.

@pepopowitz pepopowitz enabled auto-merge (squash) July 27, 2022 20:55
@pepopowitz pepopowitz merged commit d8353f3 into main Jul 27, 2022
@pepopowitz pepopowitz deleted the berndruecker-patch-2 branch July 27, 2022 21:15
@berndruecker
Copy link
Member Author

berndruecker commented Jul 28, 2022

Thanks @pepopowitz, for fixing the formatting!
I understand your concerns of being part of the process - and I agree it should not be necessary. At the same time, I would love to reduce the barrier for people to contribute to the docs - especially small fixes or improvements that can easily be created as PR via the Github UI by anybody. Obstacles like this would bring the experience down. And honestly: Nobody who only wants to make a small improvement in the docs will read contribution guidelines - I would be surprised.

PS: I think I don't share the opinion about the importance of consistency in source files (that does not surface for the user) - but actually, I also don't have to ;-) I am just concerned if this wish for consistency affects user experience too much (which is not yet the case - as we do not get PR's of any people on a daily basis).

@pepopowitz
Copy link
Collaborator

I would love to reduce the barrier for people to contribute to the docs - especially small fixes or improvements

I'm 100% in agreement with this. I see this current issue as a temporary bump in the road, which we will resolve with robots. I apologize for the inconvenience, and thank you for your patience until we get there.

@pepopowitz
Copy link
Collaborator

Nobody who only wants to make a small improvement in the docs will read contribution guidelines - I would be surprised.

I do when I submit these kinds of PRs to projects, so we're out there 😅

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.

3 participants