This repository has been archived by the owner on Oct 23, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 40
Glasscat tests #152
Merged
Merged
Glasscat tests #152
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d3197a2
remove old docstring function
alfredclwong 825f621
remove ambiguities and unbound_args tests
alfredclwong 49e64e5
using OpticSim.GlassCat
alfredclwong c81d47d
export CARGILLE
alfredclwong 28a92b5
prettier isapprox tests
alfredclwong 8feca2a
prettier unitful usage
alfredclwong d6d588a
Air.jl, GlassTypes.jl, search.jl testsets
alfredclwong edf90a8
comment out findglass test
alfredclwong 3a94c1c
use exported variables directly
alfredclwong a55e1fc
add comment explaining why @test findglass is commented
alfredclwong eb8016a
Merge branch 'main' into glasscat-tests
alfredclwong 97cc2e5
glassnames() tests
alfredclwong 0e15ddd
using Unitful.DefaultSymbols
alfredclwong d9bb38a
add wrappedallocs macro, fixing allocation test issues
alfredclwong fb3f1fc
Merge branch 'main' into glasscat-tests
alfredclwong 9f2a7d5
Merge branch 'main' into glasscat-tests
alfredclwong File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the docstring function being permanently deleted or did you just move it somewhere? Even though we're not using it much now it seems to be functioning ok. Other people might find it useful for making tools to analyze glasses. Is it necessary to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it out because it seemed like duplicate code (I can't remember where from), it was unused, and it was lowering our test coverage.
Just had a look and it seems to be essentially the same as
info()
above but without the padding. We should really have a third function to handle the data extraction such thatinfo()
anddocstring()
only need to handle the string formatting.The other thing that confused me was that we weren't even using
docstring()
to generate the docstrings for GlassCat - we had an abbreviated version seen ingenerate.jl > glassinfo_to_docstring
. This is essentially a triple duplicate - what do you think we should keep?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically what it did - just printed the same stuff as info with correct formatting for documentation generation - I don't see much need for it if we don't have glass docs any more.
I think glass catalog jl files get written with short form information for tooltips in VS Code, this function was used specifically for long form information for use in generation of the documentation website
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrianGun you can have the final say - if you want to keep the code then feel free to
git revert d3197a2