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

Miscelanneous cleanups #2283

Merged
merged 7 commits into from
Oct 4, 2024
Merged

Miscelanneous cleanups #2283

merged 7 commits into from
Oct 4, 2024

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Oct 3, 2024

What does this PR do?

Just a couple of minor cleanups and improvements I've accumulated while working on the codebase.

How does this PR change Premake's behavior?

None intended.

@tritao tritao marked this pull request as ready for review October 3, 2024 08:53
Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

As annoying as it might be to submit these individually, I think that might have been the better option. Happy to keep working within the one PR though, but up to you.

Comment on lines +296 to +297
int has_error_prefix = strncmp(message, "** Error:", 9) == 0;
printf(has_error_prefix ? "%s\n" : ERROR_MESSAGE, message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly

Suggested change
int has_error_prefix = strncmp(message, "** Error:", 9) == 0;
printf(has_error_prefix ? "%s\n" : ERROR_MESSAGE, message);
int has_error_prefix = strncmp(message, "** Error:", 9) == 0;
printf(ERROR_MESSAGE, has_error_prefix ? message + 9 : message);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was essentially my first idea for the fix, except I did it from the Lua side.

However I decided to keep it consistent with other warning and info messages which have the "**" prefix. So I don't think your suggestion is the way to go, either we just get rid of it in Lua side, or deal with it as I did.

I think I'll keep it like this for consistency with other Premake Lua messages, but if we don't mind getting rid of the "**" I can further clean everything in a second PR.

@tritao
Copy link
Contributor Author

tritao commented Oct 3, 2024

I left the docs around for configuration so people have a reference for old scripts.

@tritao tritao force-pushed the cleanups branch 2 times, most recently from 9fdec82 to ea6566f Compare October 3, 2024 21:38
@tritao
Copy link
Contributor Author

tritao commented Oct 3, 2024

As annoying as it might be to submit these individually, I think that might have been the better option. Happy to keep working within the one PR though, but up to you.

If you don't mind, I'll leave this one as-is as I spent enough time on this already. But I'll keep it in mind for the next ones and figure out a better workflow, I think using the GH tool it's possible to open PRs directly from the CLI, which should ease the pain a bit.

@samsinsane
Copy link
Member

But I'll keep it in mind for the next ones and figure out a better workflow, I think using the GH tool it's possible to open PRs directly from the CLI, which should ease the pain a bit.

I've been using TortoiseGit for about a decade now, I'd recommend checking it out if you haven't already. It integrates with the Windows shell, but I generally just operate out of the log window when doing git operations. After pushing it provides a link to create a PR but this should be pretty standard now since I'm pretty sure it's part of the server response message now.

@samsinsane samsinsane merged commit 5e1530f into premake:master Oct 4, 2024
17 checks passed
@tritao
Copy link
Contributor Author

tritao commented Oct 4, 2024

But I'll keep it in mind for the next ones and figure out a better workflow, I think using the GH tool it's possible to open PRs directly from the CLI, which should ease the pain a bit.

I've been using TortoiseGit for about a decade now, I'd recommend checking it out if you haven't already. It integrates with the Windows shell, but I generally just operate out of the log window when doing git operations. After pushing it provides a link to create a PR but this should be pretty standard now since I'm pretty sure it's part of the server response message now.

I used TortoiseGit for years, it's really nice and used to love it. Had to find something else for Linux and I've been using https://git-cola.github.io/ and been pretty happy with it.

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