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.

Sign up for GitHub

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

Jump to bottom

APS Upload Pipeline Polish #1048

Merged
merged 3 commits into from
Jul 25, 2024
Merged

APS Upload Pipeline Polish #1048

merged 3 commits into from
Jul 25, 2024

Conversation

BrandonPacewic
Copy link
Member

@BrandonPacewic BrandonPacewic commented Jul 25, 2024 •
edited
Loading

  • Now no longer throws a message box error if the user is not signed in.
    • This would previously prevent the event handlers from being added properly and would cause some undefined behaviour.
  • Will now prompt for login on export with upload and missing APS login data instead of throwing an error message.
    • Previously would parse the whole robot before telling you that your destination was invalid.
  • Removed the log that was logging the user auth token.
  • Now calling the logging module from AARD-1735: Logging module updates #1010.

JIRA Issue

@BrandonPacewic BrandonPacewic self-assigned this Jul 25, 2024
@BrandonPacewic BrandonPacewic requested review from HunterBarclay and a team as code owners July 25, 2024 18:48
@BrandonPacewic BrandonPacewic requested review from PepperLola and azaleacolburn and removed request for a team July 25, 2024 18:48
azaleacolburn
azaleacolburn approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@azaleacolburn azaleacolburn left a comment

Choose a reason for hiding this comment

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

LGTM

exporter/SynthesisFusionAddin/src/Parser/SynthesisParser/Parser.py
Comment on lines 186 to +187
else:
assert self.exporterOptions.exportLocation == ExportLocation.DOWNLOAD
Copy link
Member

Choose a reason for hiding this comment

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

I forgot that enums could be undefined 🤦‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm more so just checking to make sure we don't add different export locations in the future. Sort of a "this was the entire behaviour of ExportLocation when I wrote this code so it may need to be updated" type of thing. But yea I suppose it will also catch if it is None as well.

exporter/SynthesisFusionAddin/src/APS/APS.py
curr_time = time.time()

currTime = time.time()
if os.path.exists(auth_path):
Copy link
Member

@azaleacolburn azaleacolburn Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should it display a messagebox or smth if the path is invalid?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking not. Just means that the user is not currently logged in / has never logged in before. But this could be up for debate.

Copy link
Member

Choose a reason for hiding this comment

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

omg I'm dumb, I was thinking of the download path oml, totally out of the scope of this PR + smth that won't ever be undefined. 🤦‍♀️

PepperLola
PepperLola approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@PepperLola PepperLola left a comment

Choose a reason for hiding this comment

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

Looks good

@HunterBarclay HunterBarclay merged commit df36e28 into dev Jul 25, 2024
13 checks passed
@HunterBarclay HunterBarclay deleted the branp/1774/polish-aps-pipeline branch July 25, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azaleacolburn azaleacolburn azaleacolburn approved these changes

@PepperLola PepperLola PepperLola approved these changes

@HunterBarclay HunterBarclay Awaiting requested review from HunterBarclay HunterBarclay is a code owner

Assignees

@BrandonPacewic BrandonPacewic

Labels
aps exporter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BrandonPacewic @PepperLola @azaleacolburn @HunterBarclay