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

Remove the internal prefix for value fields #647

Merged
merged 2 commits into from
Jan 27, 2022
Merged

Remove the internal prefix for value fields #647

merged 2 commits into from
Jan 27, 2022

Conversation

tefra
Copy link
Owner

@tefra tefra commented Jan 26, 2022

📒 Description

The originalCase convention performs no string transformations when used for field/classes/... names.
The generator is also using the field name @value for all infered text nodes.

These two don't work very well together.

Resolves #646

🔗 What I've Done

In the past enumeration classes were allowed to be inner|nested. To resolve that issue I used the prefix @ for value fields in order to quickly identify these cases when the generator had to copy inner classes from one class to another.

It's being a while now that all enumeration classes are promoted to root classes, so that hack to identify these cases is no longer necessary so it's safe to remove that part of the code and also remove the @ prefix that is causing the issue #646

💬 Comments

Hopefully most of these hacks from the early days are long gone.

🛫 Checklist

tefra added 2 commits January 26, 2022 21:19
Notes:
In the past enumeration classes were allowed
to be inner|nested. To resolve that issues
I had used the prefix @ for value fields
in order to quickly identify these cases.

I know it was an ugly hack but worked well
till now that someone noticed the conflict
with the originalCase name convention.

Since that part of the code was hackish
and not really used anymore we can get rid
for the code and the silly prefix.
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #647 (077e077) into master (b8c50fe) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #647   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        94           
  Lines         8214      8212    -2     
  Branches      1584      1583    -1     
=========================================
- Hits          8214      8212    -2     
Impacted Files Coverage Δ
xsdata/codegen/utils.py 100.00% <ø> (ø)
xsdata/codegen/handlers/class_extension.py 100.00% <100.00%> (ø)
xsdata/models/xsd.py 100.00% <100.00%> (ø)
xsdata/utils/constants.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8c50fe...077e077. Read the comment docs.

@tefra tefra merged commit f4d73b1 into master Jan 27, 2022
@tefra tefra deleted the issue-646 branch January 27, 2022 17:23
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.

Imports raise SyntaxError with FieldName as originalCase
1 participant