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

Enhanced New Dataframe dialog #7964

Merged

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Nov 15, 2022

Fixes #7957
@berylwaswa can you test this? Thanks.

rdstern
rdstern previously approved these changes Nov 16, 2022
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.

@N-thony I am approving, because it does solve the problem that we found. So it could be merged.
But there are also problems with the defaults of letters(1,10) and LETTERS(1,10) that you may want to resolve at the same time.
There are 2 problems with those options:
a) when they are used they currently only work if the length of the data frame is a multiple of 10. It usually will be, but if you put the usual rep round it, then it would always work.
b) They won't work, when the variable is numeric or integer. So check if they are set. I suggest if they are set and the type is numeric or integer, you make that variable into character instead.
c) Also they won't work if the type is set to be a factor and you have set the levels. So, again, check that and make it a character variable instead.

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@N-thony Thanks, just 1 small comment. Will you also make the changes requested by @rdstern?

Comment on lines 603 to 604
Dim iColDefaultIndex As Integer = dataTypeGridView.CurrentRow.Cells("colDefault").ColumnIndex
Dim iRowDefaultIndex As Integer = dataTypeGridView.CurrentRow.Cells("colDefault").RowIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

these are only used on line 608.
Move these lines into the line 606 If block.

@N-thony
Copy link
Collaborator Author

N-thony commented Nov 21, 2022

@N-thony Thanks, just 1 small comment. Will you also make the changes requested by @rdstern?

@lloyddewit I will make them later.

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.

Fine

@lloyddewit lloyddewit changed the title Minor improvement in New Dataframe dialogue Enhanced New Dataframe dialog Nov 22, 2022
@lloyddewit lloyddewit merged commit 41faefc into IDEMSInternational:master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the New Data Frame dialogue
3 participants