-
Notifications
You must be signed in to change notification settings - Fork 110
Added in "Save Definitions" Option to End of Rain/Seasons Dialog for PICSA #10049
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Vitalis95, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the 'End of Rain/Seasons Dialog' by providing users with the ability to save their defined rain and season parameters. This feature improves the usability and reproducibility of analyses within the PICSA application by allowing users to persist and reuse their custom definitions, rather than re-entering them for each session. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a 'Save Definitions' option to the End of Rain/Seasons dialog. The changes include modifications to the UI designer file to add the new controls and updates to the code-behind to implement the logic for saving the definitions. The implementation looks mostly correct, but I've found a couple of issues. One is a potentially script-breaking bug due to a typo in a variable name, and the other is a minor UI text issue that could confuse users. My review includes suggestions to fix these issues.
instat/dlgEndOfRainsSeason.vb
Outdated
| clsGetCalculationsFunction.SetAssignTo("calculations_data") | ||
|
|
||
| clsGetOffsetTermFunction.SetRCommand(frmMain.clsRLink.strInstatDataObject & "$get_offset_term") | ||
| clsGetOffsetTermFunction.SetAssignTo("definitions_offset ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instat/dlgEndOfRainsSeason.vb
Outdated
| ucrSaveObject.SetPrefix("end_rain_definition") | ||
| ucrSaveObject.SetSaveType(strRObjectType:=RObjectTypeLabel.StructureLabel, strRObjectFormat:=RObjectFormat.Text) | ||
| ucrSaveObject.SetDataFrameSelector(ucrSelectorForWaterBalance.ucrAvailableDataFrames) | ||
| ucrSaveObject.SetLabelText("Survival Object Name:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label text for ucrSaveObject is set to "Survival Object Name:", which seems unrelated to saving rain definitions and might be a copy-paste artifact. A more descriptive label like "Definition Object Name:" would be clearer for the user, especially since the object prefix is set to end_rain_definition.
ucrSaveObject.SetLabelText("Definition Object Name:")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I ran this it ran for get_end_rains_definition:
end_rain_definition <- get_end_rains_definition(summary_data, calculations_data, end_rains_date=end_season_date, definitions_offset)We want to have end_rains_date equal the name of the variable in the ucrSave (e.g., "end_rains_date")
It also is missing out the end_rains parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some very minor changes
instat/dlgEndOfRainsSeason.vb
Outdated
| ucrChkEndofSeasonDoy.SetLinkedDisplayControl(grpEndofSeason) | ||
| ucrNudTotalOverDays.SetLinkedDisplayControl(lblTotalOverDays) | ||
|
|
||
| ucrSaveObject.SetPrefix("end_rain_definition") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be end_rains_definition, just to be consistent with the other naming conventions in the other saves
instat/dlgEndOfRainsSeason.vb
Outdated
| If ucrChkDefinitions.Checked Then | ||
| ucrBase.clsRsyntax.AddToAfterCodes(clsGetEndRainDefFunction, iPosition:=13) | ||
| If ucrChkEndofSeasonDate.Checked Then | ||
| clsGetEndRainDefFunction.AddParameter("end_rains_date", ucrInputEndofSeasonDate.GetText, iPosition:=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I think this is where the error is. This should be ucrInputEndofRainsDate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you want them in quotes, i.e.,
clsGetEndRainDefFunction.AddParameter("end_rains_date", Chr(34) & ucrInputEndofRainsDate.GetText & Chr(34), iPosition:=2)
instat/dlgEndOfRainsSeason.vb
Outdated
| If ucrChkEndofSeasonDate.Checked Then | ||
| clsGetEndRainDefFunction.AddParameter("end_rains_date", ucrInputEndofSeasonDate.GetText, iPosition:=2) | ||
| ElseIf ucrChkEndofSeasonDoy.Checked Then | ||
| clsGetEndRainDefFunction.AddParameter("end_rains", ucrInputSeasonDoy.GetText, iPosition:=3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ucrInputRainsDoy and in quotes
instat/dlgEndOfRainsSeason.vb
Outdated
| clsGetEndRainDefFunction.AddParameter("end_rains", ucrInputSeasonDoy.GetText, iPosition:=3) | ||
|
|
||
| ElseIf ucrChkEndofSeasonOccurence.Checked Then | ||
| clsGetEndRainDefFunction.AddParameter("end_rains_status", ucrInputEndofSeasonOccurence.GetText, iPosition:=4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again here for the End of Rains, not Season (and quotes)
|
@lilyclements , have a look at it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vitalis95 good - only a couple of minor things
- I made a minor fix in the parameter names for get_end_season_definition so can you pull your changes?
- If it is end of season, then the base function is "get_end_season_definition"
(You did "get_end_rains_definition" for them both) - If it is end of season, can the ucrSave say "end_season_definition"
|
@lilyclements , check it again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vitalis95 this is crashing for me for the end season option. Is it working for you?
@lilyclements I get this error
|

Fixes partially #10041
@lilyclements @berylwaswa , I have implemented Save Definitions" Option to the End of Rain/Seasons Dialog. Please check