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

Update hidden files flag #56

Merged
merged 15 commits into from
Aug 6, 2024
Merged

Update hidden files flag #56

merged 15 commits into from
Aug 6, 2024

Conversation

jayamala17
Copy link
Collaborator

@jayamala17 jayamala17 commented Aug 5, 2024

Comment on lines 65 to 70
ValidationResult.new(is_valid: is_valid, error_message: nil)
else
ValidationResult.new(
is_valid: is_valid,
error_message: @bag.errors.full_messages.join(", ")
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be this instead? Not sure.

Suggested change
ValidationResult.new(is_valid: is_valid, error_message: nil)
else
ValidationResult.new(
is_valid: is_valid,
error_message: @bag.errors.full_messages.join(", ")
)
ValidationResult.new(
is_valid: is_valid,
error_message: is_valid ? nil : @bag.errors.full_messages.join(", ")
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is_valid would be false always so wont be nil. I feel we dont need this check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I was wondering whether we could just use the conditional with error_message. My suggestion might be messed up, hold on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like this, remove the outer conditional, just use a ternary with message. https://github.com/mlibrary/bag-courier/pull/56/files#r1705968354

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can leave it the way it is, just wondering what you thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry i got confused with unified lines. Now i got it. I think it will more readable and concise to have it this way which will handle both the condition.

test/test_bag_adapter.rb Outdated Show resolved Hide resolved
test/test_bag_courier.rb Outdated Show resolved Hide resolved
lib/bag_adapter.rb Outdated Show resolved Hide resolved
@jayamala17 jayamala17 merged commit 6bebf92 into main Aug 6, 2024
1 check passed
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.

2 participants