DevToys icon indicating copy to clipboard operation
DevToys copied to clipboard

SOLID design for format converter tool

Open youkai95 opened this issue 3 years ago • 4 comments

I suggested this pattern a few days ago and I decided to give it a try and close #247. This implementation have some details that could be improved and I would like to work on them together. I hope we could walk to a really SOLID solution that could be compatible with any tool with minor effort.

Pull request type

Please check the type of change your PR introduces:

  • [ ] Bugfix
  • [x] Feature
  • [x] Code style update (formatting, renaming)
  • [x] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [ ] Internationalization and localization
  • [ ] Other (please describe):

Issue Number: #247

What is the new behavior?

  • A SOLID solution for format converter tool
  • Easily extensible to more formats

Other information

  • I would like to begin with an issue that made me expend the latest 30 minutes struggling with it. I have no idea why when the view loads, the format combo boxes does not show the currently selected (or default) format.
  • Dont blame me... I dont know where FontIcon glyphs are listed XD
  • The way the services are configured does not makes me happy... I would like to share ideas with you to get a sharper design. At the moment it seems ugly to have to put something like Converters.ConfigureService(value.Value, (service) => service.SetSerializerConfigurations(ConverterConfiguration.Indentation, IndentationMode.Value)); everywhere. In addition to that, we should think about how we could make a better use of the deserializer configurations. Maybe now it would be useless but for future features it could be necessary.

Quality check

Before creating this PR, have you:

  • [x] Followed the code style guideline as described in CONTRIBUTING.md
  • [ ] Verified that the change work in Release build configuration
  • [ ] Checked all unit tests pass

youkai95 avatar Feb 01 '22 04:02 youkai95

Hello, Thank you for the contribution, with @veler we need to review these changes as they will modifies lot's of code in the DevToy. So this pull request will take some times. Sorry.

btiteux avatar Feb 06 '22 14:02 btiteux

Hello, Thank you for the contribution, with @veler we need to review these changes as they will modifies lot's of code in the DevToy. So this pull request will take some times. Sorry.

I was aware of that, so take the time you need and let me know about any changes the code needs. Also, I would like to point out that this code has an issue in the view that I am not sure about how to solve it (the first dot in "Other Information")

youkai95 avatar Feb 06 '22 17:02 youkai95

Few days ago I took a time to review this PR and I decided to change some things. I think I could make it simpler using other pattern...

youkai95 avatar Mar 23 '22 01:03 youkai95

Is this still under active review/consideration? I'm also considering a large structural change and it would be best to work on a source code base with the other large structure change already in.

jamescurran avatar Aug 16 '22 18:08 jamescurran