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

enhancement: JSON variable file support #4542

Merged
merged 4 commits into from
Mar 10, 2023
Merged

enhancement: JSON variable file support #4542

merged 4 commits into from
Mar 10, 2023

Conversation

sunday2
Copy link
Contributor

@sunday2 sunday2 commented Nov 22, 2022

Closes #4532

@sunday2
enhancement: JSON variable file support
8845400 
Closes robotframework#4532
@sunday2
enhancement: JSON variable file support
b6f8fd8 
Closes robotframework#4532
@sunday2
Merge remote-tracking branch 'origin/enhancement_4532' into enhanceme…
2584708 
…nt_4532

# Conflicts:
#	atest/robot/variables/json_variable_file.robot
#	atest/testdata/variables/json_variable_file.robot
@sunday2 sunday2 marked this pull request as ready for review November 22, 2022 14:51
pekkaklarck
pekkaklarck requested changes Feb 13, 2023
View reviewed changes
Copy link
Member

@pekkaklarck pekkaklarck left a comment

Choose a reason for hiding this comment

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

Thanks for a great PR and sorry for reviewing taking so long. Only real issue I noticed in code and tests was that the extension wasn't case-insensitive as it should be, but I added also some other comments. Another thing to do is writing documentation. It can be done after merging this PR as well.

atest/robot/variables/json_variable_file.robot Outdated Show resolved Hide resolved
atest/testdata/variables/invalid.json Outdated Show resolved Hide resolved
atest/testdata/variables/json_variable_file.robot Outdated Show resolved Hide resolved
src/robot/variables/filesetter.py Outdated Show resolved Hide resolved
src/robot/variables/filesetter.py Show resolved Hide resolved
@sunday2
Copy link
Contributor Author

sunday2 commented Mar 5, 2023 •
edited

Thanks for a great PR and sorry for reviewing taking so long. Only real issue I noticed in code and tests was that the extension wasn't case-insensitive as it should be, but I added also some other comments. Another thing to do is writing documentation. It can be done after merging this PR as well.

Thank you for the review and all the comments, I have updated the code. If there is any other concern, pls don't hesitate to let me know. For writing document, maybe I need to spend some time on studying how to write it. BTW, could you pls tell me which document i need to update.

image

@sunday2 sunday2 requested a review from pekkaklarck March 5, 2023 02:53
@pekkaklarck pekkaklarck merged commit b9b8720 into robotframework:master Mar 10, 2023
@pekkaklarck
Copy link
Member

Thanks for a great PR! I can write the documentation myself, I think it can mainly refer to existing YAML variable file documentation so it's a small task.

yanne pushed a commit that referenced this pull request Mar 18, 2023
@sunday2 @yanne
Add JSON variable file support (#4542)
97893a1 
Implements #4532. Documentation still missing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pekkaklarck pekkaklarck Awaiting requested review from pekkaklarck

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON variable file support
2 participants
@sunday2 @pekkaklarck