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

refactor(IO): clean up ffAppendFDBuffer #671

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

apocelipes
Copy link
Contributor

for 2 reasons:

  • Simplify the Code.
  • ffAppendFDBuffer can't make assumptions about how the read data will be processed, so the trim calls are inappropriate. All callers currently handle whitespace and newlines at the end of files themselves. So these trims are also redundant and can simply be removed.

for 2 reasons:

- Simplify the Code.
- ffAppendFDBuffer can't make assumptions about how the read data will
  be processed, so the trim calls are inappropriate.
@apocelipes
Copy link
Contributor Author

Comment out that ffStrbufEnsureFixedLengthFree will take care of NUL.

@CarterLi CarterLi merged commit e85a4b9 into fastfetch-cli:dev Dec 22, 2023
18 checks passed
@CarterLi
Copy link
Member

I think you are correct. Thanks

@CarterLi
Copy link
Member

image
image

Maybe need some fixes @apocelipes

CarterLi added a commit that referenced this pull request Dec 22, 2023
@CarterLi
Copy link
Member

It turns out that there are so many trailing line endings in sysfs. I fixed some of them, but surely not all. Although the change is right, it can be too dangerous.

@apocelipes
Copy link
Contributor Author

Yes. There are 40+ calls need to be checked. Maybe we should bring trims back and add a todo: "remove this after all check finished".

@apocelipes
Copy link
Contributor Author

@CarterLi I need to apologize for not checking the code of some modules carefully. Now I did a full code review and created a new PR: #673 . Please take a look.

@apocelipes apocelipes deleted the refactor-clean-up-io branch December 25, 2023 00:38
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.

2 participants