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: add fully qualified Names for method parameters, return types, new constructor call and generic types #318

Merged
merged 22 commits into from
Apr 13, 2023

Conversation

chIsmaeel
Copy link
Contributor

@chIsmaeel chIsmaeel commented Apr 9, 2023

Resolved #298

  • Changed type names to fully qualified names
  • Updated method parameter type names
  • Updated method return type names
  • Updated new constructor calls
  • Updated generic type names
  • Updated static type names
  • Updated related test cases to reflect the changes

Old Generated Code

#nullable enable
namespace A.Payments
{
    public partial class Mapper
    {
        public partial A.Payments.InvoiceDto ToDto(Payments.Invoice invoice)
        {
            var target = new A.Payments.InvoiceDto();
            return target;
        }
    }
}

New Generated Code

#nullable enable
namespace A.Payments
{
    public partial class Mapper
    {
        public partial global::A.Payments.InvoiceDto ToDto(global::Payments.Invoice invoice)
        {
            var target = new global::A.Payments.InvoiceDto();
            return target;
        }
    }
}

Please review and provide feedback on the changes. Thank you!

@chIsmaeel chIsmaeel changed the title fix: closes #298 fix: global usings are not supported (colliding namespaces)global usings are not supported (colliding namespaces) #298 Apr 9, 2023
@chIsmaeel chIsmaeel changed the title fix: global usings are not supported (colliding namespaces)global usings are not supported (colliding namespaces) #298 fix: global usings are not supported (colliding namespaces)global usings are not supported (colliding namespaces) closes #298 Apr 9, 2023
@chIsmaeel chIsmaeel changed the title fix: global usings are not supported (colliding namespaces)global usings are not supported (colliding namespaces) closes #298 fix: global usings are not supported (colliding namespaces) #298 Apr 9, 2023
@chIsmaeel chIsmaeel changed the title fix: global usings are not supported (colliding namespaces) #298 fix: global usings are not supported (colliding namespaces) https://github.com/riok/mapperly/issues/298#issue-1644053890 Apr 9, 2023
@chIsmaeel chIsmaeel changed the title fix: global usings are not supported (colliding namespaces) https://github.com/riok/mapperly/issues/298#issue-1644053890 fix: global usings are not supported (colliding namespaces) #298 Apr 9, 2023
@chIsmaeel chIsmaeel marked this pull request as draft April 9, 2023 15:35
@chIsmaeel chIsmaeel marked this pull request as ready for review April 9, 2023 15:36
Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. The same needs to be applied at other places, not only parameters, doesn't it?
Eg. Return types, new constructor calls, generic types, ...
Also all test assertions need to be updated (not only the snapshots, but also TestHelper.GenerateMapper(... calls).
Could you adjust your commit message to not only refer to the issue but include a short description of what is beeing fixed. This shows up in the release notes.

@chIsmaeel chIsmaeel changed the title fix: global usings are not supported (colliding namespaces) #298 fix: add global usings are supported #298 Apr 11, 2023
@chIsmaeel chIsmaeel changed the title fix: add global usings are supported #298 fix: add fully qualified Names for method parameters, return types, new constructor call and generic types Apr 11, 2023
@chIsmaeel
Copy link
Contributor Author

Hey @latonz

Hi there! I've made a contribution by committing changes to the codebase. I would greatly appreciate it if you could review my changes and provide feedback.

I have thoroughly tested the changes and believe they improve the overall functionality and quality of the project. However, I am open to any suggestions or changes that you may have to further enhance the code.

Here's a brief overview of the changes I made:

  • Change type name to fully qualified names
    • Method Parameter Type Names
    • Method Return Type Names
    • new Constructor Call
    • Generic Type Names
    • Static Type Names
  • Update related Test Cases

Please let me know if there are any additional changes or improvements you would like me to make. Thank you!

Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.
You may also need to update the integration tests.

src/Riok.Mapperly/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Riok.Mapperly/Helpers/SymbolExtensions.cs Outdated Show resolved Hide resolved
@chIsmaeel chIsmaeel requested a review from latonz April 12, 2023 17:34
@chIsmaeel
Copy link
Contributor Author

chIsmaeel commented Apr 12, 2023

Thanks for the updates. You may also need to update the integration tests.

I resolved test issues

Please let me know if there are any other additional changes or improvements you would like me to make. Thank you!

Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

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

Thanks for the adjustments 😊 A few smaller feedbacks, looks good otherwise 👍

src/Riok.Mapperly/Emit/SyntaxFactoryHelper.cs Outdated Show resolved Hide resolved
src/Riok.Mapperly/Emit/SyntaxFactoryHelper.cs Outdated Show resolved Hide resolved
src/Riok.Mapperly/Emit/SyntaxFactoryHelper.cs Outdated Show resolved Hide resolved
@chIsmaeel
Copy link
Contributor Author

Thanks for feedback. I updated code according to your feedback.

Please let me know if there are any other additional changes or improvements you would like me to make. Thank you!

@chIsmaeel
Copy link
Contributor Author

Resolved all integration test issues

Please let me know if there are any other additional changes or improvements you would like me to make. Thank you!

Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

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

There are a few linter issues. Some of them can be fixed with dotnet format and verified with dotnet format --verify-no-changes && dotnet build -c Release others may need manual fixes.

@chIsmaeel
Copy link
Contributor Author

Resolved all issues

Please let me know if there are any other additional changes or improvements you would like me to make. Thank you!

@chIsmaeel chIsmaeel requested a review from latonz April 13, 2023 16:44
latonz
latonz previously approved these changes Apr 13, 2023
@latonz
Copy link
Contributor

latonz commented Apr 13, 2023

Could you squash your commits together in one feature commit according to the conventional commits specification? The title of the PR should do the job as commit message.

There are some more minor linter errors to be fixed.

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #318 (4c3a9e4) into main (dce4b42) will increase coverage by 0.06%.
The diff coverage is 93.77%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #318      +/-   ##
==========================================
+ Coverage   91.71%   91.78%   +0.06%     
==========================================
  Files         109      117       +8     
  Lines        3379     3724     +345     
  Branches      438      506      +68     
==========================================
+ Hits         3099     3418     +319     
- Misses        189      209      +20     
- Partials       91       97       +6     
Impacted Files Coverage Δ
...appings/NewInstanceObjectFactoryPropertyMapping.cs 100.00% <ø> (ø)
...escriptors/Mappings/ObjectPropertyMethodMapping.cs 90.00% <ø> (ø)
...riptors/MappingBuilders/QueryableMappingBuilder.cs 68.42% <68.42%> (ø)
...Descriptors/Mappings/EnumFromStringParseMapping.cs 73.68% <73.68%> (ø)
...criptors/Mappings/PropertyMappings/PropertyPath.cs 89.06% <80.00%> (-3.60%) ⬇️
src/Riok.Mapperly/Helpers/SymbolExtensions.cs 86.56% <81.81%> (+0.85%) ⬆️
...iptors/MappingBuilders/DictionaryMappingBuilder.cs 79.51% <83.33%> (+0.50%) ⬆️
src/Riok.Mapperly/Descriptors/WellKnownTypes.cs 87.23% <83.87%> (-8.92%) ⬇️
...ers/NewInstanceObjectPropertyMappingBodyBuilder.cs 80.43% <91.66%> (+2.83%) ⬆️
...iptors/MappingBuilders/EnumerableMappingBuilder.cs 93.49% <92.42%> (+0.23%) ⬆️
... and 39 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@chIsmaeel chIsmaeel requested a review from latonz April 13, 2023 18:06
@chIsmaeel
Copy link
Contributor Author

Resolved all lint issues

Please let me know if there are any other additional changes or improvements you would like me to make. Thank you!

@latonz latonz enabled auto-merge (squash) April 13, 2023 18:23
@latonz latonz disabled auto-merge April 13, 2023 18:23
@latonz latonz merged commit 36830e8 into riok:main Apr 13, 2023
@github-actions
Copy link

🎉 This PR is included in version 2.8.0-next.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

global usings are not supported (colliding namespaces)
3 participants