linutil
linutil copied to clipboard
ChrisTitusTech
Went over the codebase to find small problems
I just went over the codebase, finding and fixing small issues.
They were not bugs (except one that I wrote previously), but they were weird little things needing fixing.
Biggest things are:
- I made sure that yaml entry_type is a proper enum, and not a struct hack with ifs checking it. This fix DOESN'T change the toml format in any way. I suspect the author of the code didn't know about
#[serde(flatten)], which is understandable, I didn't know either until very recently :) - I fixed an issue I introduced, where typing on any keyboard with any symbols except english, would produce problems inside linutil. That was a bug and I fixed it
@JustLinuxUser Can you squash some of these commits?
just squash merge?
Chris doesn't always do that, It'd be best to squash manually to avoid cluttering the commit history.
@JustLinuxUser Can you squash some of these commits?
just squash merge?
Chris doesn't always do that, It'd be best to squash manually to avoid cluttering the commit history.
I understand that, but I am not sure he will want to apply all commits, and the commit messages are very good in this particular case, and explain the commit itself
@JustLinuxUser Can you squash some of these commits?
just squash merge?
Chris doesn't always do that, It'd be best to squash manually to avoid cluttering the commit history.
I understand that, but I am not sure he will want to apply all commits, and the commit messages are very good in this particular case, and explain the commit itself
Fair point.
@JustLinuxUser Can you squash some of these commits?
just squash merge?
Chris doesn't always do that, It'd be best to squash manually to avoid cluttering the commit history.
I understand that, but I am not sure he will want to apply all commits, and the commit messages are very good in this particular case, and explain the commit itself
Fair point.
I love the reply ladder :)
The build script options are very intentional. If you don't rebuild on change to src/commands, updating any tab or script will have no impact unless you manually delete the binary first. Cargo does not 'recursively check directories', it checks for changes to modules. No module is contained within src/commands (src/commands.rs or src/commands/mod.rs would have to exist for that to be the case), and therefore, Cargo will not check for changes to the directory or any file within. If you don't rerun the build script on change to src/, the date displayed within the program will be incorrect running subsequent builds on different days. Check #224 for information about that.
Good catch with serde flatten. I figured there was a better way to do that.
Drop https://github.com/ChrisTitusTech/linutil/pull/247/commits/9f48e2466f58d53f925ce8dbe41f53c3e856effc and fix the missing import and this will be ready to merge.
The build script options are very intentional. If you don't rebuild on change to src/commands, updating any tab or script will have no impact unless you manually delete the binary first. Cargo does not 'recursively check directories', it checks for changes to modules. No module is contained within src/commands (src/commands.rs or src/commands/mod.rs would have to exist for that to be the case), and therefore, Cargo will not check for changes to the directory or any file within. If you don't rerun the build script on change to src/, the date displayed within the program will be incorrect running subsequent builds on different days. Check #224 for information about that.
Good catch with serde flatten. I figured there was a better way to do that.
Drop https://github.com/ChrisTitusTech/linutil/pull/247/commits/9f48e2466f58d53f925ce8dbe41f53c3e856effc and fix the missing import and this will be ready to merge.
Could this be solved by utilizing [[package.include]] in the pkg config?
Yes, that solves it. Good call. The build script still must be rerun on changes to src/ though, since it's responsible, rather than the build process as a whole, for gathering the date.
The build script options are very intentional. If you don't rebuild on change to src/commands, updating any tab or script will have no impact unless you manually delete the binary first. Cargo does not 'recursively check directories', it checks for changes to modules. No module is contained within src/commands (src/commands.rs or src/commands/mod.rs would have to exist for that to be the case), and therefore, Cargo will not check for changes to the directory or any file within. If you don't rerun the build script on change to src/, the date displayed within the program will be incorrect running subsequent builds on different days. Check #224 for information about that.
Good catch with serde flatten. I figured there was a better way to do that.
Drop 9f48e24 and fix the missing import and this will be ready to merge.
I know it's intentional, but It still works without it, I checked yesterday and just checked again, modifying any toml / sh file will still force a recompile
Yes, that solves it. Good call. The build script still must be rerun on changes to src/ though, since it's responsible, rather than the build process as a whole, for gathering the date.
Have you actually had that issue, or did you just think that such an issue might happen? If you actually had such an issue, pls share your setup (cargo version, rustc version, OS, editor + plugins)
Have you actually had that issue
I changed my system clock a day forward, modified the code, and rebuilt; the date displayed was the date from the previous build until I cleaned the directory.
My toolchain version was out of date (1.80). I can't replicate either of these issues now, on 1.81. I'm not sure where the change was, or if there was just a bug somewhere (either neovim's save, or more likely cargo, has failed to rebuild my projects after changes dozens of times; I've never bothered to get to the root of the problem), but regardless, I drop my request for that commit to be modified.
The handle passthrough function has more room for improvement. We don't have to deal with a mutable variable, the logic can be compressed into a single match statement without impacting readability. Also, remember to use into_x() functions rather than to_owned() on a reference, or slice in this case, whenever said function is available and you don't need the initial value anymore. https://github.com/lj3954/linutil/commit/412e76edf4115f742b8941040d68d52d2ddd571e
Sorry for the inconvenience. We had a massive restructure of the codebase to improve future development. Because of this can you update your PR to the new structure. Thank you for your assistance and contribution.
