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

Replacement PR for #1349 (neuro images ls -l support) #1354

Closed
wants to merge 53 commits into from

Conversation

olegstepanov
Copy link
Contributor

Had to recreate PR because Azure failed to run the checks

@lgtm-com
Copy link

lgtm-com bot commented Feb 26, 2020

This pull request introduces 1 alert when merging b91d973 into 2a9fb8b - view on LGTM.com

new alerts:

  • 1 for Unused import

Copy link
Contributor

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM on green.

I think there may be some issues with formatter.

tests/cli/test_ftable.py Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review February 26, 2020 16:48
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2df131d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1354   +/-   ##
=========================================
  Coverage          ?   89.16%           
=========================================
  Files             ?       48           
  Lines             ?     6580           
  Branches          ?     1042           
=========================================
  Hits              ?     5867           
  Misses            ?      554           
  Partials          ?      159

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2df131d...008ed50. Read the comment docs.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Please add CHANGELOG.D record

neuromation/api/parsing_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

the diff does not look good. should be rebased.


class LongImagesFormatter(BaseImagesFormatter):
def __call__(self, images: Iterable[RemoteImage]) -> Iterable[str]:
rows = [[str(image), image.as_docker_url] for image in images]
Copy link
Contributor

Choose a reason for hiding this comment

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

The last neat: please add the table header, like for neuro ps command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be good headers? 'IMAGE URL', 'DOCKER URL'?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess Neuro URL and Docker URL (both capitalized) -- to reflect the fact that the first value should be used with neuro, the second is for docker.

@olegstepanov olegstepanov deleted the long-images-list-2 branch March 2, 2020 16:57
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.

4 participants