Skip to content

Conversation

hazeled
Copy link
Contributor

@hazeled hazeled commented Sep 29, 2022

A new PR with the correct branch. Adds "TCOD_printf_RGB", "TCOD_printn_RGB", and "TCOD_vprintf_RGB" (Along with a PrintParams struct) to console_printing.

@hazeled
Copy link
Contributor Author

hazeled commented Sep 29, 2022

I'm busy right now, but when I get back at my home computer I'll fix the issues you posted about in #132

@HexDecimal
Copy link
Collaborator

Your last commit reverted console_printing.c entirely.

@HexDecimal HexDecimal self-requested a review September 29, 2022 21:33
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #133 (c31f6ca) into main (38110b9) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   18.58%   18.82%   +0.23%     
==========================================
  Files         126      126              
  Lines       13540    13580      +40     
==========================================
+ Hits         2517     2557      +40     
  Misses      11023    11023              
Impacted Files Coverage Δ
src/libtcod/console_printing.c 39.47% <100.00%> (+2.33%) ⬆️
tests/test_printing.cpp 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hazeled
Copy link
Contributor Author

hazeled commented Sep 30, 2022

Haha, in my defense - that DOES technically remove usage of "TCOD_console_print_internal". But that's on me for trying to push while distracted, fixed. I'm going to go ahead and get everything else done tonight, too

@HexDecimal HexDecimal self-assigned this Sep 30, 2022
@HexDecimal
Copy link
Collaborator

The code wasn't formatted with clang-format, and and I fixed some issues which could've been found at compilation. I've pushed these changes to your branch.

What tools are you using to code with?

@hazeled
Copy link
Contributor Author

hazeled commented Sep 30, 2022

I use neovim and coc+clangd. What issues should I have caught at compile time? It compiled and ran perfectly when I tested it on my machine

@HexDecimal
Copy link
Collaborator

What issues should I have caught at compile time?

The new functions in headers ended with _RGB but those functions were called in the source as _rgb.

@hazeled
Copy link
Contributor Author

hazeled commented Sep 30, 2022

What issues should I have caught at compile time?

The new functions in headers ended with _RGB but those functions were called in the source as _rgb.

Huh. My branch is (and was) reading as up-to-date, and it compiled with both of them being _rgb. They're also both still _rgb on my system. I think something might be wrong with my git install, I'm being careful about how I add/commit/push, and it seems like some things are either not being tracked or not being pushed. My apt has also been acting up lately, too, so I'm guessing I just need to fix my system

@HexDecimal
Copy link
Collaborator

I'd recommend getting the clang-format Vim plugin so that code formatting is automatic.

Depending on how you work with Git it can be easy to get confused over what's staged. A command like git commit -a is less prone to accidentally leaving out unchanged files as long as there's no new ones.

@HexDecimal
Copy link
Collaborator

What remains are tests. I'll probably finish this PR.

@HexDecimal HexDecimal linked an issue Sep 30, 2022 that may be closed by this pull request
@hazeled
Copy link
Contributor Author

hazeled commented Sep 30, 2022

Got it, I'll set it up. Also I'd be more than happy to finish up the tests if you haven't already

@HexDecimal
Copy link
Collaborator

I've already added tests. Right now I'm just unsure if the params should be passed by value, when typically these structs are passed by pointer.

@HexDecimal HexDecimal merged commit a36785f into libtcod:main Sep 30, 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.

Printing API needs a rework.
2 participants