Skip to content

Allow changing the path to refine.ini via refine.sh/bat on command line#4220

Merged
wetneb merged 2 commits intoOpenRefine:masterfrom
tejasrbhat:tejasrbhat-4113-custom-refineini-bat
Jan 22, 2022
Merged

Allow changing the path to refine.ini via refine.sh/bat on command line#4220
wetneb merged 2 commits intoOpenRefine:masterfrom
tejasrbhat:tejasrbhat-4113-custom-refineini-bat

Conversation

@tejasrbhat
Copy link
Copy Markdown
Member

Fixes #4113

@tejasrbhat tejasrbhat self-assigned this Oct 17, 2021
@github-actions github-actions Bot added the Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements. label Oct 17, 2021
Copy link
Copy Markdown
Member

@thadguidry thadguidry left a comment

Choose a reason for hiding this comment

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

Tested on Windows. refine-dev.ini still overrides even when /c flag was used, which is expected handling. Great Job @tejasrbhat !

Someone else will need to test on Linux before we can approve and merge.

@tejasrbhat
Copy link
Copy Markdown
Member Author

@thadguidry I ran bash script in debug mode and the flow is correct. Do you know anyone who can test it ? @wetneb ?

Copy link
Copy Markdown
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

Nice! But I am not sure why refine-dev.ini should always have priority even when another filename is passed on the command line? Why not just do what the user wants and use the file they specify? @thadguidry you are generally in favour of giving full control to users, no?

Comment thread refine Outdated
@thadguidry
Copy link
Copy Markdown
Member

thadguidry commented Oct 18, 2021

@wetneb yes but thought since I currently use the dev file in stash and pop as needed it was as convenient enough but yeah probably easier to just command line for that also

@tejasrbhat
Copy link
Copy Markdown
Member Author

tejasrbhat commented Oct 18, 2021

@thadguidry @wetneb I think we can completely remove the logic for refine-dev.ini. If we need to run with dev config we can pass refine-dev.ini path. If its not passed it will take refine.ini in the default path anyway. I don't think we need to maintain default for refine.ini and refine-dev.ini both. What's your thought ? Let me know, I can go ahead and do the changes.

@wetneb
Copy link
Copy Markdown
Member

wetneb commented Oct 18, 2021

That would be fine with me too.

@thadguidry
Copy link
Copy Markdown
Member

thadguidry commented Oct 18, 2021

@tejasrbhat The refine-dev.ini is convenient in that I create one in my local repo path and then I can just use my IDE functionality without typing or pasting a command line. The idea of the -dev file was that it was exactly for that...when your in development and need to repeatedly test things typically using your IDE.

The idea of the -c config file parameter was for NOT DEV and for Users instead who often need to have specific settings depending on the kinds of workflow or project-based work they might be doing that day or week or month.

But using the -dev file with the -c config parameter is OK for me (I will likely save the launch command for my IDE, which all of them support that method for runtime testing a Java application - the typical command launch scenario is usually java -jar xxxx but it can be anything like our refine.bat -c xxxx

So yeah, fine to remove the logic currently for refine-dev.ini default.

@tejasrbhat tejasrbhat force-pushed the tejasrbhat-4113-custom-refineini-bat branch from 9fb3d59 to cf2bc53 Compare October 18, 2021 13:25
@tejasrbhat
Copy link
Copy Markdown
Member Author

@thadguidry @wetneb done the changes. I debugged the bash script for both custom and default, and flow looks fine. Attaching the screenshots for the same.

refineIniDebug

refineIniDebugCustomPath

@tejasrbhat
Copy link
Copy Markdown
Member Author

tejasrbhat commented Oct 19, 2021

@thadguidry I will look into it

@wetneb wetneb changed the title #4113 Allow changing the autosave period via refine.sh/bat on command line Allow changing the path to refine.ini via refine.sh/bat on command line Jan 18, 2022
@tejasrbhat tejasrbhat force-pushed the tejasrbhat-4113-custom-refineini-bat branch from cf2bc53 to c144e02 Compare January 22, 2022 16:36
@tejasrbhat
Copy link
Copy Markdown
Member Author

@wetneb @thadguidry sorry for fixing this so late. Was busy with personal work. I have fixed the issue with double quotes and tested. @thadguidry you can test it once and let me know if anything else needs to be added.

Comment thread refine.bat
@tejasrbhat tejasrbhat force-pushed the tejasrbhat-4113-custom-refineini-bat branch from 6d9dee5 to 3062770 Compare January 22, 2022 17:42
Copy link
Copy Markdown
Member

@thadguidry thadguidry left a comment

Choose a reason for hiding this comment

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

@tejasrbhat tested and now works great! Thanks so much for adding this command flag! (and pushing yourself to help get it finished! We thank you!)

@wetneb wetneb merged commit 07a0447 into OpenRefine:master Jan 22, 2022
j-sal pushed a commit to j-sal/OpenRefine that referenced this pull request Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow changing the autosave period via refine.sh/bat on command line

3 participants