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

fix: Write and read enum setting as string not number #8494

Merged
merged 1 commit into from Oct 1, 2020
Merged

fix: Write and read enum setting as string not number #8494

merged 1 commit into from Oct 1, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 29, 2020

Fixes #8493

Proposed changes

  • Write and read enum setting as string not number

Screenshots

Before

  • Enum value is stored as number

After

  • Enum value is stored as string

Test methodology

  • Automation

Test environment(s)

  • Git Extensions 3.4.3.9999
  • Build d4b0f48
  • Git 2.28.0.windows.1
  • Microsoft Windows NT 10.0.19041.0
  • .NET Framework 4.8.4220.0
  • DPI 96dpi (no scaling)

✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost self-assigned this Sep 29, 2020
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #8494 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8494      +/-   ##
==========================================
- Coverage   54.75%   54.74%   -0.01%     
==========================================
  Files         898      898              
  Lines       64251    64246       -5     
  Branches    11440    11474      +34     
==========================================
- Hits        35179    35171       -8     
  Misses      26390    26390              
- Partials     2682     2685       +3     
Flag Coverage Δ
#production 41.62% <100.00%> (+0.03%) ⬆️
#tests 94.82% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ghost
Copy link
Author

ghost commented Sep 30, 2020

During testing, I found out that it is impossible to save the space symbol.

writer.WriteAttributeString("xml", "space", null, "preserve");

Solves this issue.

If this is not possible. Perhaps it is worth pointing out and add some description?

You can test by setting the value.

autocrlf = input

@ghost ghost requested a review from RussKie September 30, 2020 20:47
Comment on lines +187 to +188
return JsonConvert
.DeserializeObject<T>(value);
Copy link
Member

Choose a reason for hiding this comment

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

I must have totally zoned out when reviewed your previous PR, what do we serialize/deserialize to JSON? All the settings files I've seen were all XML and XML within XML...

Copy link
Author

Choose a reason for hiding this comment

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

If value is object, now it is a structure, besides standard types.
This does not affect the xml structure.

Copy link
Author

Choose a reason for hiding this comment

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

This also allows you to collect settings into one logical group.

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to establish - is it a new or an existing functionality?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed convertation for simple types. Relatively new.

Copy link
Author

Choose a reason for hiding this comment

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

For Int, Byte, Float, Enum... changed to use type converter

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 1, 2020
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 1, 2020
@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 1, 2020
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 1, 2020
@RussKie
Copy link
Member

RussKie commented Oct 1, 2020

During testing, I found out that it is impossible to save the space symbol.

IIRC, null, empty string or a space are all treated like unset value. In what usecase do you feel we need to persist a space?

@ghost
Copy link
Author

ghost commented Oct 1, 2020

IIRC, null, empty string or a space are all treated like unset value. In what usecase do you feel we need to persist a space?

I see the space symbol as a value from a theoretical point of view. Ok I remove this.

@RussKie
Copy link
Member

RussKie commented Oct 1, 2020

I see the space symbol as a value from a theoretical point of view. Ok I remove this.

Right. In this case let's remove it until we have a real usecase where we need this.
It may worth adding a test that fails, and thus document this behaviour.

@ghost
Copy link
Author

ghost commented Oct 1, 2020

VS allows to store space symbols. 😄

The test exists. When I delete it will fail.

@ghost
Copy link
Author

ghost commented Oct 1, 2020

image

For char ' ' also.

I'll have to remove them for the build to work.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

I'm going to accept it since we need to fix the bug.

But in the near future I'd like to revisit JSON serialization, which I understand is new. The Settings are on the hot path, since they are used quite liberally throughout the app.
Loading and unloading settings also affects how quickly the Settings dialog opens up too (and it is very slow as is).

I don't think we require persistence of objects right now, we generally persist strings and integral types. There are few scenarios where we persist collections, but those are persisted as XML separately and then saved as strings.

@RussKie RussKie merged commit 534dbf1 into gitextensions:master Oct 1, 2020
@ghost ghost added this to the 4.0 milestone Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script settings: JsonReaderException
2 participants