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

fix: remove column command in favor of awk #721

Merged
merged 6 commits into from
Jun 16, 2020

Conversation

homburg
Copy link
Contributor

@homburg homburg commented May 8, 2020

  • asdf plugin list
  • asdf plugin list all

Avoid hard dependency on column command. This command
is not included in a fresh ubuntu container and not
defined in posix:

https://pubs.opengroup.org/onlinepubs/9699919799/idx/utilities.html

Also makes the output of asdf plugin list all line-streamed
on the second column value

@homburg homburg requested a review from a team as a code owner May 8, 2020 11:04
@homburg homburg changed the title Fix crashes for Fix crashes for missing column command May 8, 2020
@Stratus3D
Copy link
Member

It looks like one of the tests failed because of the change in command output. Can you correct the failing test?

@Stratus3D
Copy link
Member

Why not just get rid of column entirely and just always use awk?

@homburg
Copy link
Contributor Author

homburg commented May 9, 2020

Column has an advantage in auto-sizing the first column. Awk just has a hardcoded size for the first column.

I can remove the column call if you prefer.

@jthegedus
Copy link
Contributor

After looking into many options for this solution, I think we should remove column and extract the awk scripts into our own format file for us to manage our screen output as functions in their own place.

Adding deps to asfd is difficult (though we are pre v1.0 so now would be the time). We're already using awk so as long as we keep awk script to POSIX compliant than we should be good.

If the content changes to require further modifications to column padding it shouldn't be too difficult to update. Annoying, yes, but less so than getting all users and systems to rely on another dep.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Pending input from @Stratus3D I would like to see column removed.

Though I am happy to see this get merged as a stopgap if we require a longer discussion around dependencies.

Print aligned columns without non-posix
column command
Handle missing non-posix column command gracefully
@homburg
Copy link
Contributor Author

homburg commented Jun 14, 2020

@jthegedus Thanks for the feedback, I've removed the column calls

@jthegedus
Copy link
Contributor

@Stratus3D are we okay with merging this, and then having a follow-up PR that addresses converting all other tabular output to awk and potentially word-wrapping?

@@ -20,7 +20,7 @@ plugin_list_all_command() {

printf "%s\\t%s\\n" "$index_plugin_name" "$installed_flag$source_url"
done
) | column -t -s $'\t'
) | awk '{ printf("%-28s", $1); sub(/^[^*]/, " &", $2); $1=""; print $0 }'
Copy link
Member

Choose a reason for hiding this comment

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

Why do these two awk commands differ? The column commands they replace are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one:

  1. prints the first column at 28 char width, left-aligned
  2. Adds a space to column 2's without leading "*" for alignment
28-chars-wide                 next-column
28-chars-wide                *column-with-asterisk

The other one just prints the first column at 28 char width, left-aligned

Copy link
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

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

Unsure why the awk commands are different. I think the command should be the same in both cases. Otherwise these changes look good to me.

@Stratus3D
Copy link
Member

Note to whoever merges: probably best to squash and merge this PR.

@Stratus3D Stratus3D requested a review from jthegedus June 16, 2020 15:29
@homburg
Copy link
Contributor Author

homburg commented Jun 16, 2020

@jthegedus Are your change requests addressed or do you have further requests?

@jthegedus
Copy link
Contributor

@homburg You've addressed them, was just waiting to see your response to @Stratus3D 's questions. LGTM too, will merge

@jthegedus jthegedus changed the title Fix crashes for missing column command fix: remove column command in favor of awk Jun 16, 2020
@jthegedus jthegedus merged commit 1b7b4da into asdf-vm:master Jun 16, 2020
@homburg homburg deleted the fix/no-column-command branch August 3, 2020 07:11
@jthegedus jthegedus added this to the v0.8.x milestone Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants