Skip to content

[WIP] Apply automatic board list formatting from #10111 to Makefiles#10130

Closed
x3ro wants to merge 1 commit intoRIOT-OS:masterfrom
x3ro:pr/consistently-format-board-lists
Closed

[WIP] Apply automatic board list formatting from #10111 to Makefiles#10130
x3ro wants to merge 1 commit intoRIOT-OS:masterfrom
x3ro:pr/consistently-format-board-lists

Conversation

@x3ro
Copy link
Copy Markdown
Contributor

@x3ro x3ro commented Oct 8, 2018

Contribution description

Applies the formatting mechanism from #10111 to all relevant Makefiles. Still marked as WIP since there are some corner cases to be fixed.

@x3ro x3ro added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Oct 8, 2018
@x3ro x3ro requested a review from MrKevinWeiss October 8, 2018 12:41
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 8, 2018

am I the only one who's bothered by having to scroll down first thing after opening a test's Makefile?

jnohlgard
jnohlgard reviewed Oct 8, 2018
View reviewed changes
tests/socket_zep/Makefile Outdated
native \
on \
only \
socket_zep
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Oct 8, 2018

Choose a reason for hiding this comment

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

Oops!

Copy link
Copy Markdown
Contributor Author

@x3ro x3ro Oct 8, 2018

Choose a reason for hiding this comment

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

See #10111

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Oct 8, 2018

@kaspar030

am I the only one who's bothered by having to scroll down first thing after opening a test's Makefile?

Not sure what you mean? Do you mean that the board blacklist/whitelists are too long and you have to scroll down to see the USEMODULE etc lines in the Makefile?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 8, 2018

Do you mean that the board blacklist/whitelists are too long and you have to scroll down to see the USEMODULE etc lines in the Makefile?

Yes...

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 8, 2018 •
edited
Loading

am I the only one who's bothered by having to scroll down first thing after opening a test's Makefile?

So far yes @kaspar030, or at least it seems like the opinions on the trade-off between diff cleanliness and long board lists are favoring the former

@waehlisch
Copy link
Copy Markdown
Member

waehlisch commented Oct 8, 2018

You can just search: /USEMODULE

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 8, 2018

You can just search: /USEMODULE

Yup. Everytime I open the file, which might be hundred times more often than I modify the lists. Multiplied by the number of people that read at our code, that is quite often.

To me it seems like the board lists get much more screen real estate as they are relevant.
See this vs this.

If there's a script that does the formatting anyways, why not have that output lines that fill 80 characters?

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Oct 8, 2018

Maybe we just move the BOARD_* below the USEMODULE?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 8, 2018

Maybe we just move the BOARD_* below the USEMODULE?

Yeah, why not? as far down as possible.

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 8, 2018

If there's a script that does the formatting anyways, why not have that output lines that fill 80 characters?

To make diffs clean, relevant and reviewable.

Maybe we just move the BOARD_* below the USEMODULE?

Seems doable

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Oct 16, 2018

What about including from (a) separate files(s)?

@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 16, 2018

What about including from (a) separate files(s)?

Could you elaborate how this would improve the situation over putting the lists at the end of the file?

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Oct 23, 2018

Could you elaborate how this would improve the situation over putting the lists at the end of the file?

I didn't intend to compete with the "end of file" solution. :) It was just a suggestion to get that list out of the sight.

At least you can retain the original loading order when you use includes. And if there are intentions to do any processing (formatting) it can prevent messing up other parts. But I'm not sure if any of this really matters.

@x3ro x3ro force-pushed the pr/consistently-format-board-lists branch from 9669152 to e3cb71f Compare November 11, 2018 10:04
@x3ro x3ro force-pushed the pr/consistently-format-board-lists branch from e3cb71f to be08fa0 Compare November 11, 2018 10:06
@x3ro x3ro mentioned this pull request Nov 11, 2018
Closed
4 tasks
MrKevinWeiss
MrKevinWeiss reviewed Nov 12, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

So far only a few things that should be fixed up manually then I am happy.

tests/mpu_stack_guard/Makefile
CFLAGS += -Wno-infinite-recursion
endif

BOARD_WHITELIST := \
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss Nov 12, 2018

Choose a reason for hiding this comment

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

Maybe manually alter this so it doesn't look so silly? I don't it is worth the effort adding this corner case into the script.

tests/socket_zep/Makefile

TERMFLAGS ?= -z [::1]:17754

BOARD_WHITELIST := \
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss Nov 12, 2018

Choose a reason for hiding this comment

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

Also manually fix this guy.

@stale
Copy link
Copy Markdown

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Sep 10, 2019

Darn...

@MrKevinWeiss MrKevinWeiss reopened this Oct 2, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Oct 2, 2019
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Oct 2, 2019

Maybe see if the updated script works on this. If there are only a few corner cases that go against convention I think manual fixes are fine.

@x3ro x3ro closed this Oct 26, 2019
@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Oct 26, 2019

Closing this. I'd prefer to open a new PR with a clean discussion.

@x3ro x3ro mentioned this pull request Oct 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard left review comments

@MrKevinWeiss MrKevinWeiss MrKevinWeiss left review comments

Assignees

No one assigned

Labels

State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@x3ro @kaspar030 @jnohlgard @waehlisch @MrKevinWeiss @A-Paul