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

Fixes importing of Climsoft observation data using station names #9137

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

Patowhiz
Copy link
Contributor

Fixes #6986.
@rdstern this is ready for review.
This should now be able to import observations using station names that have apostrophes as well.
I've also changed the dialog title to show the version that the dialog is compatible with.
I've also removed the climsoft wizard menu item.

@rdstern
Copy link
Collaborator

rdstern commented Sep 17, 2024

@Patowhiz looks good to me. I'm not sure how to test your correction, but the menu changes look fine.
a) I don't like your title which mentions (version 4.2.2) of climsoft. This means we'll have to edit the dialog each time there is a change in climsoft? That sort of information could go into the help perhaps. If it remains then saying From Version 4.2.2, instead would be better?
b) The 2 sub-dialogs are now added to the help file - and I suggest we need to write the help soon. Can you please add the corresponding help IDs. They are 354 for the Connect sub-dialog and 2 for the More Climsoft Options sub-dialog.
c) In the Connect sub-dialog is there anything more you could add to the pull-downs, so it is easy to connect in different countries and also to your web versions - especially the Zambia one, that is part of the R-Instat library.

Then @N-thony can you check also and approve as soon as possible. It would be good to merge today, or tomorrow, as it makes changes to the main code.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Sep 17, 2024

@rdstern I've removed the 4.2.2 and added the help ids.
In regards to defaults of the connect sub dialog, I think what is there should suffice.

rdstern
rdstern previously approved these changes Sep 17, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

Great. @N-thony over to you to check and hopefully merge?

Comment on lines 5262 to 5242
Me.Text = "R-Instat " + My.Application.Info.Version.Major.ToString + "." +
My.Application.Info.Version.Minor.ToString + "." +
My.Application.Info.Version.Build.ToString
Me.Text = "R-Instat .."
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you put back the version number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@N-thony this is quite interesting, I didn't change the title text at all. I have now reverted the title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@N-thony, now I understand why it changed itself. And it would keep changing whenever design changes are done.

This kind of 'advanced' concatenation is besy done in the code file, not the designer file. Would be good if this is done in separate PR, I'm assuming you are aware of when this was added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that will solve this issue of reverting the version number. If you are happy to open a new issue about then I will assign it to @derekagorhom for the quick change.

@N-thony
Copy link
Collaborator

N-thony commented Sep 18, 2024

@rdstern back to you

@N-thony N-thony merged commit 3c7a30d into IDEMSInternational:master Sep 18, 2024
2 checks 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.

Climsoft Import Wizard Dialog to be deleted?
4 participants