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

Make class variables immutable #187

Closed

Conversation

WilliamJamieson
Copy link
Contributor

An issue pointed out in #183 is that we have many of the classes which define mutable class variables. In general, mutable class variables should not be used because the state of that variable is shared across all instances. Thus they should be made to be immutable objects.

This PR corrects this issue for this package.

Note that this issue occurs mostly for the tags and types attributes of the converters. These were turned into properties instead of tuples because ASDF does not support these values being anything other than lists (ASDF will literally segfault).

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #187 (c6608be) into main (70e27a2) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
+ Coverage   98.63%   98.71%   +0.08%     
==========================================
  Files          55       55              
  Lines        2047     2180     +133     
==========================================
+ Hits         2019     2152     +133     
  Misses         28       28              
Impacted Files Coverage Δ
asdf_astropy/converters/coordinates/angle.py 100.00% <100.00%> (ø)
...f_astropy/converters/coordinates/earth_location.py 100.00% <100.00%> (ø)
asdf_astropy/converters/coordinates/frame.py 100.00% <100.00%> (ø)
...f_astropy/converters/coordinates/representation.py 100.00% <100.00%> (ø)
asdf_astropy/converters/coordinates/sky_coord.py 100.00% <100.00%> (ø)
...f_astropy/converters/coordinates/spectral_coord.py 100.00% <100.00%> (ø)
asdf_astropy/converters/fits/fits.py 100.00% <100.00%> (ø)
asdf_astropy/converters/table/table.py 98.24% <100.00%> (+0.46%) ⬆️
asdf_astropy/converters/time/time.py 100.00% <100.00%> (ø)
asdf_astropy/converters/time/time_delta.py 100.00% <100.00%> (ø)
... and 14 more

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

This should be done because if these were edited in one instance then it would change across all instances.
@braingram
Copy link
Contributor

@WilliamJamieson Would you help me reproduce the segfault? Testing locally with several asdf version and extensions within asdf, custom extensions and ones in asdf-astropy I've so far failed to produce a segfault by changing the types and tags to tuples.

@WilliamJamieson
Copy link
Contributor Author

@WilliamJamieson Would you help me reproduce the segfault? Testing locally with several asdf version and extensions within asdf, custom extensions and ones in asdf-astropy I've so far failed to produce a segfault by changing the types and tags to tuples.

You are correct, it was an independent bug I introduced.

@braingram
Copy link
Contributor

Thanks for the follow up!

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.

2 participants