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

Adds original audio download feature #526

Merged
merged 8 commits into from
Jun 6, 2022
Merged

Adds original audio download feature #526

merged 8 commits into from
Jun 6, 2022

Conversation

atruskie
Copy link
Member

We can download recordings from the A2O or from Ecosounds.

Closes #525

Changes

Adds 5 new commands:

  • download (a stub)
  • download file
  • download repositories
  • download search
  • download batch

Issues

Still need to add unit tests.

Visual Changes

See images included in docs.

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Link any PRs or issues blocking this PR from being merged
  • Remove/Reduce warnings from edited files
  • Unit tests have been added for changes
  • Ensure CI build is passing

We can download recordings from the A2O or from Ecosounds.

Closes #525
Downgrades on dotnet restore were causing failures in the build
Copy link
Collaborator

@Allcharles Allcharles left a comment

Choose a reason for hiding this comment

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

Most of this looks fine. The services seem to be fairly well written, and most of the code using it is fairly simple. Only concern I have is the download file command which seems to have a confused use case. Giving it the ability to download multiple files using the file IDs blurs the line between it and the download batch command. There doesn't seem to be a reason to separate their functionality. This confusion has lead to weird/conflicting documentation in the manual for download file.

docs/technical/commands/download.md Outdated Show resolved Hide resolved
docs/technical/commands/download/batch.md Outdated Show resolved Hide resolved
Usage: AnalysisPrograms.exe download batch [options] <Ids>

Arguments:
Ids One or more audio files to download
Copy link
Collaborator

Choose a reason for hiding this comment

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

Audio recording ids? How do users find this information?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point, however, the shell help (for which this is) must be fairly terse. I'll add a guide that describes how to get these Ds

Comment on lines +44 to +46
-p|--project-ids <PROJECT_IDS> Project IDs to filter recordings by
-r|--region-ids <REGION_IDS> Region IDs to filter recordings by
-s|--site-ids <SITE_IDS> Site IDs to filter recordings by
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar issue, will need to tell users how to find this information

--end <END> A date (exclusive) to filter out recordings. Can parse an ISO8601 date.
-f|--flat If used will not place downloaded files into sub-folders
-o|--output <OUTPUT> A directory to write output to
-repo|--repository <REPOSITORY> Which repository to use to download audio from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Link to AnalysisPrograms.exe download repositories?

{
if (this.Repository.IsNullOrEmpty())
{
throw new CommandLineArgumentException("The repository option must be specified");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not give the repositories hint here as well?

{
if (this.AuthToken.IsNullOrEmpty())
{
this.Console.InfoLine($"An authentication token is needed to connect to {this.Repository}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Authentication hint on where to find this?

}

// https://raw.githubusercontent.com/spectreconsole/spectre.console/a690ce49556615fea49e61972646eb52a11bbdb5/src/Spectre.Console/Internal/FileSize.cs
internal struct FileSize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coming from JS, I'm surprised there is no library for this

Comment on lines +1 to +3
// <copyright file="FileSize.cs" company="QutEcoacoustics">
// All code in this file and all associated files are the copyright and property of the QUT Ecoacoustics Research Group (formerly MQUTeR, and formerly QUT Bioacoustics Research Group).
// </copyright>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this valid for this file?

{
console.WriteLine(message, Info);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

public static void HackerLine(this IAnsiConsole console, string message)
{
    console.WriteLine("I'm in: " + message, new (foreground: Color.Green, background: Color.black));
}

@atruskie atruskie merged commit 71fb73d into master Jun 6, 2022
@atruskie atruskie deleted the remote-download branch June 6, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download remote audio from repositories
2 participants