Skip to content

Conversation

@atharvRsharma
Copy link
Contributor

@atharvRsharma atharvRsharma commented Oct 28, 2025

Tried to continue nanononi's implementation, might not be what was asked of me. The seperation of concerns approach was used where ReadConfigFiles is left alone to actually apply the config file if found, as of now the configpath func finds all possible paths and returns a vector of them, printconfiginfo then checks existence of files within said path then give output to the console.

#2349

@atharvRsharma atharvRsharma marked this pull request as ready for review October 29, 2025 10:01
@mwestphal
Copy link
Member

First lets "just" rebase naninoni work, so the diff looks similar to this: https://github.com/f3d-app/f3d/pull/2382/files

@atharvRsharma atharvRsharma force-pushed the print_config_info branch 2 times, most recently from dd69c08 to c07e15e Compare November 2, 2025 04:34
@mwestphal
Copy link
Member

Need any help moving forward @atharvRsharma ?

@atharvRsharma
Copy link
Contributor Author

oh my bad, shouldve kept you updated, as of right now no i dont need help as id like to figure this out on my own but i will definitely ask if im stuck

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

changes needed

@atharvRsharma
Copy link
Contributor Author

should i push the updated version with the changes you specified with/without rebasing or do i just continue with the implementation for now and push when ive done a substantial amount of work?

@mwestphal
Copy link
Member

Hi @atharvRsharma

Need any help moving forward ?

@atharvRsharma
Copy link
Contributor Author

Hi @atharvRsharma

Need any help moving forward ?

Sorry, exams coming up, I'm still trying obviously, it'll just be a little slower, hope that's not a problem

@mwestphal
Copy link
Member

No problem, thanks for the feedback :)

@mwestphal
Copy link
Member

Hi @atharvRsharma

Need any help moving forward ?

@atharvRsharma
Copy link
Contributor Author

Hi @atharvRsharma

Need any help moving forward ?

Hi, yea I'm so sorry, I think it's best if you unassign me for now, I'll be free from the 23rd when my exams end and if the issue is still free I'll ask to be assigned then, I don't want to block any other people from working on this issue obviously.

@mwestphal
Copy link
Member

No worries, this can wait for january :)

@atharvRsharma
Copy link
Contributor Author

thank you so much for understanding.

@mwestphal
Copy link
Member

Hi @atharvRsharma

Let me know when this is ready for review :)

@atharvRsharma
Copy link
Contributor Author

Hi @atharvRsharma

Let me know when this is ready for review :)

Hi yea, it should be ready now, accidentally commented an else block i wasnt supposed to, should be fine hopefully.

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

This seems correct to me, small nitpicking changes.

I'll run CI.

@snoyer please review

@mwestphal
Copy link
Member

\ci fast

@mwestphal
Copy link
Member

please rebase on latest master @atharvRsharma

@atharvRsharma
Copy link
Contributor Author

yes of course ill get on that as soon as possible, does the main implementation seem fine? aside from the few style changes you suggested and the noexcept part

@mwestphal
Copy link
Member

I’m not entirely sure why the patch coverage check is failing again, is it expected of me to write tests for the print_config_info func? seems a little adhoc-y but then again i dont really know how patch coverage works

Dont worry about it, Ill check it out.

@atharvRsharma
Copy link
Contributor Author

thank you

@mwestphal
Copy link
Member

Too increase coverage you should be able to add a "TooLong" test for --config, see application/testing/CMakeLists.txt 1680.

@atharvRsharma
Copy link
Contributor Author

atharvRsharma commented Jan 8, 2026

Too increase coverage you should be able to add a "TooLong" test for --config, see application/testing/CMakeLists.txt 1680.

f3d_test(NAME TestTooLongConfigCLI ARGS --config config NO_RENDER NO_BASELINE LONG_TIMEOUT) does this work? should i add a comment for this?

@mwestphal
Copy link
Member

Too increase coverage you should be able to add a "TooLong" test for --config, see application/testing/CMakeLists.txt 1680.

f3d_test(NAME TestTooLongConfigCLI ARGS --config config NO_RENDER NO_BASELINE LONG_TIMEOUT) does this work? should i add a comment for this?

You definitely need a REGEX of some sort

@atharvRsharma
Copy link
Contributor Author

could you maybe explain what the regex means and does in this context? im not really aware about the concept, sorry by the way.

@mwestphal
Copy link
Member

could you maybe explain what the regex means and does in this context? im not really aware about the concept, sorry by the way.

A REGEX test would read the textual output of F3D and check a specific string is present in the output.

@atharvRsharma
Copy link
Contributor Author

atharvRsharma commented Jan 8, 2026

ohhh, ok i get it, i have this thats supposed to execute regardless of config file existence(this just checks path existence)

{
  f3d::log::info("Found available config path");
}```

so should it be something like 
```f3d_test(NAME TestTooLongConfigCLI ARGS --config config NO_RENDER NO_BASELINE LONG_TIMEOUT PASS_REGULAR_EXPRESSION "Found available config path")``` ?

@mwestphal
Copy link
Member

ohhh, ok i get it, i have this thats supposed to execute regardless of config file existence(this just checks path existence)

{
  f3d::log::info("Found available config path");
}```

so should it be something like 
```f3d_test(NAME TestTooLongConfigCLI ARGS --config config NO_RENDER NO_BASELINE LONG_TIMEOUT PASS_REGULAR_EXPRESSION "Found available config path")``` ?

Not really no, can we discuss on discord ?

@atharvRsharma
Copy link
Contributor Author

ohhh, ok i get it, i have this thats supposed to execute regardless of config file existence(this just checks path existence)

{
  f3d::log::info("Found available config path");
}```

so should it be something like 
```f3d_test(NAME TestTooLongConfigCLI ARGS --config config NO_RENDER NO_BASELINE LONG_TIMEOUT PASS_REGULAR_EXPRESSION "Found available config path")``` ?

Not really no, can we discuss on discord ?

yes of course 1 sec

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

small changes needed

@atharvRsharma
Copy link
Contributor Author

Understood

@atharvRsharma
Copy link
Contributor Author

apologies for the double commits, network went down and it didnt show the account selection popup so i thought it failed

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

small changes

@atharvRsharma
Copy link
Contributor Author

anything left for me to do? is rebasing necessary yet?

@mwestphal
Copy link
Member

@snoyer please review :)

@mwestphal
Copy link
Member

anything left for me to do? is rebasing necessary yet?

lets just give a chance for @snoyer to review :)

@atharvRsharma
Copy link
Contributor Author

anything left for me to do? is rebasing necessary yet?

lets just give a chance for @snoyer to review :)

of course, also may i ask an off-issue question?

@mwestphal
Copy link
Member

also may i ask an off-issue question?

Sure, but maybe on discord ? :)

@mwestphal
Copy link
Member

No feedback from @snoyer , merging :)

@mwestphal mwestphal merged commit c7fda63 into f3d-app:master Jan 10, 2026
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants