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: ObjectAlignment enum names #74

Merged
merged 10 commits into from
Nov 3, 2023
Merged

fix: ObjectAlignment enum names #74

merged 10 commits into from
Nov 3, 2023

Conversation

ufrshubham
Copy link
Collaborator

@ufrshubham ufrshubham commented Nov 2, 2023

Description

Name of some of the enum values of ObjectAlignment were not matching the values from Tiled. As a result, tilesets with topLeft, topRight, bottomLeft and bottomRight alignment couldn't be parsed. https://doc.mapeditor.org/en/stable/reference/tmx-map-format/#tmx-tileset

image

Another problem that was discovered after fixing the names was with the byName method from the EnumByName extension. That method was not using names from ObjectAlignmentExtension that Tiled defines. To fix that, this PR adds a byName static method for ObjectAlignment which uses the correct names.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

@jtmcdole
Copy link
Collaborator

jtmcdole commented Nov 2, 2023

Would you want to leave the enum symbols in lowerUpperCase to pass lint tests, but return the string lowercase?

@ufrshubham
Copy link
Collaborator Author

Would you want to leave the enum symbols in lowerUpperCase to pass lint tests, but return the string lowercase?

The lint warning were not for the enum change, but I made the change anyways 🙂

@ufrshubham
Copy link
Collaborator Author

ufrshubham commented Nov 3, 2023

@spydon I noticed that the pubspec.yaml was missing from the root of this project. If I am not wrong, it needs to be added because of melos. I added it in this PR, but now a lot of warnings are reported. Should I make these additional changes in a separate PR or pushing it along with this PR is fine?

Most of the warning are easily fixable, but these two seem to be from flame_lint:

warning • Warning in the included options file /opt/hostedtoolcache/flutter/3.13.9-stable/x64/.pub-cache/hosted/pub.dev/flame_lint-0.2.0+2/lib/analysis_options.yaml(122..135): The option 'implicit-casts' is no longer supported • analysis_options.yaml:1:10 • included_file_warning

warning • Warning in the included options file /opt/hostedtoolcache/flutter/3.13.9-stable/x64/.pub-cache/hosted/pub.dev/flame_lint-0.2.0+2/lib/analysis_options.yaml(148..163): The option 'implicit-dynamic' is no longer supported • analysis_options.yaml:1:10 • included_file_warning

@ufrshubham ufrshubham changed the title fix!: ObjectAlignment enum values fix: ObjectAlignment enum values Nov 3, 2023
@ufrshubham ufrshubham marked this pull request as ready for review November 3, 2023 10:30
@ufrshubham ufrshubham changed the title fix: ObjectAlignment enum values fix: ObjectAlignment enum names Nov 3, 2023
@spydon
Copy link
Member

spydon commented Nov 3, 2023

@ufrshubham aah yes, that is needed for the "new" Melos version. You can fix them in this PR if it is easier for you. If you upgrade flame_lint to 1.1.1 the warning should go away.

/// Returns the [ObjectAlignment] based on given [name].
///
/// Throws an [ArgumentError] if no match is found.
static ObjectAlignment byName(String name) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static ObjectAlignment byName(String name) {
static ObjectAlignment fromName(String name) {

Maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to fromName

@spydon
Copy link
Member

spydon commented Nov 3, 2023

Haha aah, unlucky with one line going past 80 now 😅

@spydon spydon enabled auto-merge (squash) November 3, 2023 13:32
@spydon spydon merged commit 628f1f6 into flame-engine:main Nov 3, 2023
5 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.

None yet

3 participants