xdg-ninja
xdg-ninja copied to clipboard
Modifications that improve scriptability of code
I apologize for the rather-large scope of this merge, if there are any particular features that are not suitable for this project let me know and I can remove them from the merge request.
Features added:
- Changed help parsing to use case instead of if statements.
- Intelligent color detection, and ``--color=` flag
- Do not apply color when being piped into a TTY.
- Do not apply color if user has the "$NO_COLOR" environment variable set.
- ``--color=
flag which acts in the same way it does for programs such as
lswith
auto(default),
always, and
never` options.
- Added ``--decoder=` flag which allows users to define their own. (These two features should close #73)
- Flag for json output to allow for the user to parse the output in a shell script more easily.
- Flag to skip warnings to clean up output, which closes #93
- exit code dependent upon amount of ERRs logged, which closes #94
I'm quite happy with most of the changes you've made here, but there are a few issues.
I won't have time to properly look into all of this now, I'll probably get to it tomorrow.
Here's what I've noticed so far:
- The checks for missing XDG vars don't output colours (I get a literal
{ANSI_BEGIN}${ANSI_BOLD}${ANSI_CYAN}${ANSI_END}The $XDG_STATE_HOME environment variable is not set, make sure to add it to your shell's configuration before setting any of the other environment variables!${ANSI_BEGIN}${ANSI_BOLD}${ANSI_CLEAR}${ANSI_END}
in the output) - The json output option outputs multiple json objects, instead of one large json array. This is not something you necessarily have to take care of, having it like this is better than not having it at all, but it should make it easier to process this output.
- (There probably shouldn't be any other output when passing the json flag. Again, better like this than not at all, but would make it easier to process the output.) Since this would require even more fundamental changes to the script, let's not address it in this PR.
- I'm not sure about how you're handling ansi codes, it makes it easier to read for those of us who haven't memorized the codes, but I think there might be better ways to do this. This is also something you don't have to handle directly for this to be merged, but the shellcheck warnings do worry me, even though it should be fine.
Overall, this is really cool, thank you! I'll get back to this tomorrow.
Another current issue with this MR is that most of the options I added are dependent on order.
xdg-ninja.sh --help --color=never
Will not work as expected but
xdg-ninja.sh --color=never --help
will.
MR now closes #94, (I will fix all of the minor bugs in a single commit once I have all the major stuff out of the way,)
Sorry, but I don't want to use a configuration file, I don't think it's necessary, and it adds unnecessary clutter.
Again, you don't need to rewrite the whole script in this PR, it's more than enough if the features you added work, I can do the argument parsing and colour codes myself later.
Sorry, I'll undo that commit.
All issues mentioned should be resolved now
MR now closes #94, (I will fix all of the minor bugs in a single commit once I have all the major stuff out of the way,)
In theory, there may be more than 255 files
MR now closes #94, (I will fix all of the minor bugs in a single commit once I have all the major stuff out of the way,)
In theory, there may be more than 255 files
I think we can just ignore that and return 255 for 255 or more files.
In theory, there may be more than 255 files
I think we can just ignore that and return 255 for 255 or more files.
Yes
I think the scope of this PR is far too large to be properly reviewable, so I switched to using variables for ANSI codes in 79e71eaef3ab8cb2e8ec479a54f735c5b19056e2.
I'd also propose that we stick to the current version of the argument parsing using if statements for now, I'm open to addressing this later.
That should make this much more manageable, and hopefully we'll get it merged soon.
I'll see if I can resolve the merge conflicts later myself. Hope you're on board with this, if you have any thoughts let me know.
I fixed the merge conflicts, found a few issues along the way.
- The decoder option doesn't seem to do anything
- The json output option outputs the entire json file, even though some files aren't actually present, I believe @Layerex had already pointed this out too
- The json output option emits errors at the beginning (second one is only present if it's been run before):
mkdir: cannot create directory ‘/run/user/1000/xdg-ninja/’: File exists
Input error: Is a directory
I'm beginning to think it might be better to split this up into multiple PRs, since I think this introduces more complexity than is necessary.
Let me know what you think.
- The json output option emits errors at the beginning (second one is only present if it's been run before):
mkdir -p
should be used.
JSON output should be done completely differently. Writing to file is redundant.