Skip to content

Switching from FileSize to DataSize for logback size-based properties. #15957

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

Closed
wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Feb 14, 2019

see gh-15930.
DataSize can contain a negative value, Should we handle this? If yes, should we throw an exception or use a default value?
Thanks

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 14, 2019
@snicoll
Copy link
Member

snicoll commented Feb 15, 2019

@nosan thanks for the PR. Did you check if the FileSize string format works transparently with DataSize?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 15, 2019
@nosan
Copy link
Contributor Author

nosan commented Feb 15, 2019

Thanks, @snicoll
I have added some tests to check this here and here.

Current implementation parses the value via DataSize class, if something wrong, then it tries to parse a value using FileSize class.

P.S. one case which is not covered is a negative value. I think the exception should be thrown as a value is not valid.

Let me know please if I missed something.

@nosan
Copy link
Contributor Author

nosan commented Feb 16, 2019

FYI @snicoll,
I changed the implementation this morning and added more tests. Instead of stupid parsing via both classes, I added a regex to handle both formats.

@snicoll
Copy link
Member

snicoll commented Feb 19, 2019

@nosan than you for the PR but I've decided to go with a different approach, see eee07ef for more details.

@snicoll snicoll closed this Feb 19, 2019
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 19, 2019
@nosan nosan deleted the gh-15930 branch February 19, 2019 10:26
@nosan
Copy link
Contributor Author

nosan commented Feb 19, 2019

@snicoll sure, no problem.
BTW, just to make sure, you did nothing with a negative value, is it ok?

@snicoll
Copy link
Member

snicoll commented Feb 19, 2019

I noticed your comment about that. Yes I think it is ok as the main purpose was to align tooling and size format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants