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

feature: Add default path of /etc/rc.conf when not using any arg #10

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

gyptazy
Copy link
Contributor

@gyptazy gyptazy commented Dec 5, 2024

feature: Add default path of /etc/rc.conf when not using any arg

  • Use default path of /etc/rc.conf when not using any arg
  • Validate that default path exists in case of execution on any other system

Fixes: #9

@scovl
Copy link
Owner

scovl commented Dec 27, 2024

Hey @gyptazy, thanks for the PR! It looks great for handling the /etc/rc.conf default and checking if the file exists on other systems. There’s just one small snag: the CI fails because there’s no newline at the end of main.c. With -Werror, that tiny warning becomes a full-blown error. Just add a quick newline (press Enter at the end of the file), and it should be all good!

A couple of other small tips:

  1. Watch out for pointer arithmetic:
    // Wrong (this is pointer addition)
    file_path + default_file;
    
    // Right (actual assignment)
    file_path = default_file;
  2. If you don’t specifically need access(file_path, R_OK), you can just rely on fopen() failing if the file doesn’t exist or isn’t readable.

Anyway, thanks again for your work—looking forward to merging this once the newline fix is in!

 * Use default path of `/etc/rc.conf` when not using any arg
 * Validate that default path exists in case of execution on any other system
@gyptazy gyptazy force-pushed the feature/9-add-default-path branch from edd36b5 to 9e54cfa Compare December 27, 2024 21:31
@gyptazy
Copy link
Contributor Author

gyptazy commented Dec 27, 2024

Hey @scovl,

thanks, just added and squashed.

Thank you very much for the other tips, really appreciate! Feel free to edit this PR to fit your needs and acceptance criteria!

Thanks,
gyptazy

@scovl scovl merged commit e72f53c into scovl:main Dec 28, 2024
1 check passed
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.

Feature: Use default path of /etc/rc.conf
2 participants