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

prefix with package name for foreign names #35

Merged
merged 2 commits into from Sep 12, 2018
Merged

prefix with package name for foreign names #35

merged 2 commits into from Sep 12, 2018

Conversation

KristofferC
Copy link
Contributor

I was getting to know the code base but I found it quite difficult since I would look for a name and then found out it was defined in another package (like FEMBasis).

This PR adds prefixes to the names that are defined in other packages.
It also updates with the changes + fixes in JuliaFEM/FEMQuad.jl#15 (fixing names of HEX81 & co, and returning a length 1 tuple for 1 dim quadrature rules).

PR will fail until we either checkout the FEMQuad branch or tag it or something (one of the drawbacks with splitting things into multiple packages heh).

@@ -9,17 +9,16 @@ macro lintpragma(s)
end

import Base: getindex, setindex!, convert, length, size, isapprox,
start, first, next, done, last, endof, vec,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no longer defined in Base

@@ -374,11 +374,7 @@ function get_integration_points(element::Element{E}) where E
# first time initialize default integration points
if length(element.integration_points) == 0
ips = get_integration_points(element.properties)
if E in (Seg2, Seg3, NSeg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can remove special case with changes to FEMQuad.

@TeroFrondelius
Copy link
Member

I am ok with both options. There are unlimited version numbers available. Which ever is the most convenient way of working in this situation.

@ahojukka5
Copy link
Member

Good changes. Now we explicitly define where are the types defined.

Now it would be a good place to make things right, not just almost right. From a practical point of view, should we now increase a patch or a minor version in FEMQuad and what would be best practice to work with this kind of multi-package environment to avoid dependency issues? Newest FEMQuad is now 0.2.0. Add minimum version requirement for FEMQuad to 0.2.1 for FEMBase?

@KristofferC
Copy link
Contributor Author

I think what we can do is to have the packages use the master version of the dependencies, (FEMBasis, FEMQuad, FemBase) for a while and when we are ready to do release we tag all the packages.

@TeroFrondelius
Copy link
Member

@KristofferC if the master version of the package depends on the master version of other packages, it won't break anything. We just need to remember to check dependencies before tagging new version. I really like this workflow for now. I believe it will solve the problem for now.

@KristofferC
Copy link
Contributor Author

I might missunderstand you, but if a tag needs to be done for every change in a dependency then it is hard to do any experimental work since every new tag will be pushed to users.

I believe a release should be done when there is some confidence that the new stuff should be released to users but there also need to be a way to develop without tagging.

So for CI, using the master branch of FEMQuad here, until we want to make a release is what I propose.

@TeroFrondelius
Copy link
Member

Yes, exactly. I liked what you proposed. Your proposal is perfect because it won't break anything. Afterward, when we will have new features, we will tag in the right order and fix the dependencies to be tagged versions again.

Yes please, change the REQUIRE file to depend on master (or any other way which works to depend on the master version).

@KristofferC
Copy link
Contributor Author

I changed the .travis script so that we check out master of FEMQuad. Tests pass now.

@ahojukka5
Copy link
Member

I lowered a coverage threshold to avoid failing coverage check because I think coveralls is not showing coverage right for Julia 1.0 syntax at the moment. Thus it doesn't make sense to require that every line is covered. And in general, to make development work smoother, we can tolerate 1-2 uncovered lines.

Copy link
Member

@ahojukka5 ahojukka5 left a comment

Choose a reason for hiding this comment

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

These changes are ok for me.

@ahojukka5 ahojukka5 merged commit 2c1ff96 into master Sep 12, 2018
@ahojukka5 ahojukka5 deleted the kc/prefix branch May 3, 2020 12:42
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.

None yet

3 participants