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

Addressing FileUtils::ExtractExpandedPath failure #288

Merged
merged 1 commit into from
Jul 13, 2022
Merged

Addressing FileUtils::ExtractExpandedPath failure #288

merged 1 commit into from
Jul 13, 2022

Conversation

adanforth
Copy link
Contributor

Motivation

Modifications

Change summary

Added brief check on the return value of wordexp. Upon receiving a bad status code, return a sentinel value indicating failure. Also added documentation of this behavior on the function description.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

source/util/FileUtils.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@shangabl shangabl left a comment

Choose a reason for hiding this comment

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

Can you add some unit tests for Config as well?

source/util/FileUtils.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@marcoemorais-aws marcoemorais-aws left a comment

Choose a reason for hiding this comment

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

Direction is good. Some bugs and some style recommendations.

source/config/Config.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
source/main.cpp Outdated Show resolved Hide resolved
source/main.cpp Outdated Show resolved Hide resolved
source/util/FileUtils.cpp Outdated Show resolved Hide resolved
source/util/FileUtils.cpp Outdated Show resolved Hide resolved
test/util/TestFileUtils.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@marcoemorais-aws marcoemorais-aws left a comment

Choose a reason for hiding this comment

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

Looks great, just a few minor comments with the test. I will try to use it as well.

test/util/TestFileUtils.cpp Outdated Show resolved Hide resolved
test/config/TestConfig.cpp Outdated Show resolved Hide resolved
source/util/FileUtils.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
source/config/Config.cpp Outdated Show resolved Hide resolved
shangabl
shangabl previously approved these changes Jul 7, 2022
HarshGandhi-AWS
HarshGandhi-AWS previously approved these changes Jul 7, 2022
Copy link
Contributor

@HarshGandhi-AWS HarshGandhi-AWS left a comment

Choose a reason for hiding this comment

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

Update the PR title and final commit message to something more relevant. Everything else looks good to me.

Copy link
Contributor

@marcoemorais-aws marcoemorais-aws left a comment

Choose a reason for hiding this comment

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

Nice work and congrats on your first commit!

@adanforth adanforth changed the title Adanfo first CR Addressing FileUtils::ExtractExpandedPath failure Jul 7, 2022
@shangabl shangabl dismissed stale reviews from marcoemorais-aws, HarshGandhi-AWS, and themself via 7373788 July 7, 2022 23:07
@adanforth adanforth force-pushed the adanfo branch 2 times, most recently from 446839f to 703b66d Compare July 8, 2022 18:15
fixed linter issues

added logic for checking ExtractExpandedPath failures

Added/Fixed some tests and a typo

fixed test

fixed bugs and added style changes

fixed linter issues

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

fixed test

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

Added more specific error messages
@adanforth adanforth merged commit b78192f into main Jul 13, 2022
@adanforth adanforth deleted the adanfo branch July 13, 2022 20:42
joenghoyin pushed a commit to eguanatech/aws-iot-device-client-deprecated that referenced this pull request Aug 29, 2023
fixed linter issues

added logic for checking ExtractExpandedPath failures

Added/Fixed some tests and a typo

fixed test

fixed bugs and added style changes

fixed linter issues

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

fixed test

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

Added more specific error messages
joenghoyin pushed a commit to eguanatech/aws-iot-device-client-deprecated that referenced this pull request Aug 30, 2023
fixed linter issues

added logic for checking ExtractExpandedPath failures

Added/Fixed some tests and a typo

fixed test

fixed bugs and added style changes

fixed linter issues

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

fixed test

fixed repeated code, capitalization, and removed TODO comment

Removed redundant code, added formatting for error messages, updated shadow file size/readme

fixed error message

Added more specific error messages
# Conflicts:
#	source/fleetprovisioning/FleetProvisioning.cpp
#	test/config/TestConfig.cpp
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.

5 participants