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 export of ScalarValues and VectorValues #416

Merged
merged 6 commits into from
Feb 4, 2022

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Feb 4, 2022

ScalarValues and VectorValues are exported but don't seem to be defined.
I also added a test to check that no symbols are exported that haven't been defined. Not sure where to put this test though...

@fredrikekre
Copy link
Member

Removed in #404. Docs look more ugly now, it was nice to be able to reference both CellVectorValues and FaceVectorValues as VectorValues:P

@fredrikekre fredrikekre requested a review from kimauth February 4, 2022 09:00
@KnutAM
Copy link
Member Author

KnutAM commented Feb 4, 2022

Not sure why "curl" fails, the new test passes when I run ]test on my computer
Edit: I'll improve the test to solve this, seems to be an issue with qualified use...

@kimauth
Copy link
Member

kimauth commented Feb 4, 2022

We could document the VectorValued and ScalarValued Traits and refer to them in the docs instead. Though the question is where in the docs that should go, it's not really supposed to be exposed to the user.

Not sure if I like writing out CellScalarValues and FaceScalarValues everywhere, e.g. PointScalarValues uses some of these functions as well. That list easily becomes lengthy.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022

Codecov Report

Merging #416 (1198c06) into master (fb4e1f1) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #416   +/-   ##
=======================================
  Coverage   91.05%   91.06%           
=======================================
  Files          22       22           
  Lines        2897     2898    +1     
=======================================
+ Hits         2638     2639    +1     
  Misses        259      259           
Impacted Files Coverage Δ
src/Dofs/ConstraintHandler.jl 93.47% <0.00%> (+0.02%) ⬆️

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 fb4e1f1...1198c06. Read the comment docs.

@KnutAM
Copy link
Member Author

KnutAM commented Feb 4, 2022

Could one write e.g "where u is VectorValued" where the link is to a page documenting what VectorValued means (but not giving this page in the menu). And also documenting ScalarValued on that page?

@kimauth
Copy link
Member

kimauth commented Feb 4, 2022

Seems not so nice to me to hide it away like that. Perhaps just an introductory section at the top of https://ferrite-fem.github.io/Ferrite.jl/stable/reference/fevalues/ which explains that there are ScalarValues and VectorValues, where the VectorValues are zero-padded versions of the ScalarValues? I remember that I found this hard to understand in the beginning.

@KnutAM
Copy link
Member Author

KnutAM commented Feb 4, 2022

Yes, that would be even better!
Note that as far as I could find, this "problem" only occurred in the two places where I changed it.
Should that intro be done in this PR? (I'm probably not the right person at this time to write it...)

@kimauth
Copy link
Member

kimauth commented Feb 4, 2022

No need to do it in this PR, the PR is after all about removing the export that I forgot to remove in #404 . I can draft something for the VectorValues and ScalarValues.

@kimauth kimauth merged commit 085a30a into Ferrite-FEM:master Feb 4, 2022
@KnutAM KnutAM deleted the kam/removeundefexports branch February 10, 2022 23:22
koehlerson pushed a commit that referenced this pull request Apr 22, 2022
* Remove export of undefined datatypes ScalarValues and VectorValues

* Remove mentioning of undefined datatypes in docstrings

* Add test to ensure all exported symbols are defined

* Fix test to check qualified exported symbols

* Update test/runtests.jl

* Remove changes to docs, to be updated in other PR

Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
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.

4 participants