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

[pretty] Add example sources to pretty. #430

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

turon
Copy link
Contributor

@turon turon commented Apr 20, 2020

Problem

Examples are only getting style formatted via restyled during pull request right now.

Solution

This PR adds the examples sources to PRETTY_FILES so they can be checked and formatted locally during development ahead of submitting a pull request.

Helps resolve friction cited in #345.

$(NULL)

PRETTY_FILES = \
lock-app/efr32/include/AppEvent.h \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turon First time I'm digging into pretty, so sorry for the dumb question... is there a way to make this such that every developer doesn't have to remember to do this for every file added? (Re-inforces why I prefer restyled).

Maybe we can use restyled.io locally instead of make pretty? @rwalker-apple thoughts? That way we have stuff that's at least inline, and doesn't require this manual addition of stuff...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woody-apple I'm a big proponent of simplifying the developer experience and resonate with the goals you mention; unfortunately, I don't know of a simpler way to do it with autotools.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turon would you be against me moving make pretty to run this? https://github.com/restyled-io/restyler

It does exactly what our repository is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woody-apple I don't have a problem with it. @gerickson?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woody-apple I don't have a problem with it. @gerickson?

As long as it can run offline and doesn't explicitly require a Docker container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gerickson Why is offline and docker required for active development?

Trying to understand the resistance here to matching to what the repository does? Seems like a benefit to use a general system that we've deployed here rather than one that requires each developer to understand which file to edit to make sure make-pretty works.

Note: You only need to grab the container once with Docker, so technically doesn't require online.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of offline, I'd like to ensure that, as a developer, if I happen to be on a plane or somewhere where I can't consistently ping out to remote services that I can do everything I need to do to land commits locally where the push to remote is a final step I take when I land or get connectivity back. If style compliance is part of landing commits locally with confidence that I've passed all quality gates, then the tooling to do that should be local.

I may not understand restyled.io well at this point; however, my understanding is that the service is running remote tooling to do what clang-format formerly did locally. But, like travis-ci and others, it may just be triggering clang-format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restyled.io just uses clang-format, does exactly the same thing that make-pretty tries to do, but does so globally on files rather than using a developer supplied list of files to make-pretty. It also uses shell check for others, as well as markdown cleanup using 'pretty-markdown'.

I agree we need the tooling to work on the developer's machine & it's true that you need to download the docker image once, but then it's offline forever. That doesn't seem like a burden personally. Same goes for builds & VSCode development now, if you want builds to run like travis, you need to download the container once, and then you're good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a "format entire tree" as a script in integrations/ would be my preference, but is a low priority given how awesome restyled has been so far

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling the prettifying of the tree here: #432

Another PR to come later with some changes to make this easy for anyone to run.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@turon turon force-pushed the pr/pretty/examples branch from 2698964 to daaeed5 Compare April 20, 2020 18:30
@woody-apple
Copy link
Contributor

@jelderton @BroderickCarlin ?

@woody-apple woody-apple merged commit ef904ab into project-chip:master Apr 21, 2020
mkardous-silabs pushed a commit to mkardous-silabs/connectedhomeip that referenced this pull request Dec 19, 2022
Merge in WMN_TOOLS/matter from feature/MATTER_GSDK_TODO_removal to silabs_slc_1.0

Squashed commit of the following:

commit 172623b7a385f0a1a171aada81feb5901c3820cb
Author: lpbeliveau-silabs <louis-philip.beliveau@silabs.com>
Date:   Tue Dec 13 11:38:21 2022 -0500

    Removed GSDK_TODO related to matter 814
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.

None yet

5 participants