Skip to content

Update filetable.py example to make it work #55

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

Merged
merged 7 commits into from
Jan 18, 2024
Merged

Conversation

stargaser
Copy link
Collaborator

@stargaser stargaser commented Dec 9, 2023

  • Fix an import bug
  • Use firefly_url as an argument
  • Don't launch a browser if --printurl is specified
  • Pick up FIREFLY_URL environment variable if it is set
  • Insert a pause after printing the Firefly URL, before uploading the table, so a user has a chance to open their browser
  • Add option to sort the file paths
  • Add option to prepend to the file path URLs because /external is the default mounting area for Firefly-in-Docker
  • Reformat single-quotes to double-quotes to be more like black formatting
  • Use black to reformat the file

@stargaser stargaser self-assigned this Dec 9, 2023
@stargaser stargaser marked this pull request as draft December 9, 2023 01:00
    * add pause before uploading table
    * add FIREFLY_URL environment variable as the default
    * print a message while files are being pattern-matched
    * reformat single-quotes to double-quotes
@stargaser stargaser marked this pull request as ready for review December 9, 2023 01:25
@stargaser
Copy link
Collaborator Author

I'm using this to do some work for NEO Surveyor (my new position) and found a number of updates were needed.

@gpdf
Copy link

gpdf commented Jan 17, 2024

In general I think the documentation for the main() should probably include an explanation that this whole thing only works if the same directory is mounted/visible on the Firefly server.

Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

@stargaser it works great! I tested it with a local firefly server with both printurl disabled and enabled. Left minor comments, then it's ready to merge.

Also I liked that I could understand what this script does using python filetable.py --help. We should maybe document this script example in README (not necessarily in this PR though)

@gpdf
Copy link

gpdf commented Jan 18, 2024

In general I think the documentation for the main() should probably include an explanation that this whole thing only works if the same directory is mounted/visible on the Firefly server.

I think you could address this in epilog= help in the argument parser.

@stargaser
Copy link
Collaborator Author

I've added extended help to the description and the epilog. Would be great if either or both of @jaladh-singhal or @gpdf could try out filetable.py --help!

@jaladh-singhal
Copy link
Member

I've added extended help to the description and the epilog. Would be great if either or both of @jaladh-singhal or @gpdf could try out filetable.py --help!

LGTM!

@gpdf gpdf self-requested a review January 18, 2024 01:37
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Great, thank you! Looks good.

@stargaser stargaser merged commit 1b4856f into master Jan 18, 2024
@stargaser stargaser deleted the update-filetable branch January 18, 2024 02:40
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.

3 participants