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

move images to images/ and update links #3515

Closed
wants to merge 3 commits into from
Closed

Conversation

benjub
Copy link
Contributor

@benjub benjub commented Sep 24, 2023

Since we haven't made progress in the last three years on #1887, I'm at least moving all images to a subdirectory to make things a bit clearer.

Copy link
Contributor

@tirix tirix left a comment

Choose a reason for hiding this comment

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

LGTM

jkingdon
jkingdon previously approved these changes Sep 24, 2023
Copy link
Contributor

@jkingdon jkingdon left a comment

Choose a reason for hiding this comment

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

I'm fine with this change although it does come with a bit of a disclaimer in that metamath/metamath-website-seed#14 and metamath/metamath-website-scripts#2 may prevent the live site from updating.

@benjub
Copy link
Contributor Author

benjub commented Sep 24, 2023

I'm fine with this change although it does come with a bit of a disclaimer in that metamath/metamath-website-seed#14 and metamath/metamath-website-scripts#2 may prevent the live site from updating.

Therefore, merging this one will hasten those two ? ;)

@GinoGiotto
Copy link
Contributor

I fully support reorganizing the set.mm folder as I found it chaotic and confusing the first times I browsed through it.

In #1887 there is a extensive debate, but in the end I think I like the proposal from Mario. I particularly appreciate the concept of creating a separate folder for each .mm database and their own associated files (such as the discouraged files). I'm also persuaded by the suggestion of renaming this repo into metamath/databases or similar.

I'm aware that reorganizing and renaming are not risk-free, but the thing is I didn't find the research to be intuitive when I first discovered metamath. One of the first things a new user asks is: "Where are all the theorems?". For me the answer didn't come in the form of "Oh that's where they are", but rather in the form of "Set.mm? What's set.mm? Mh, it looks like there is math stuff here, so maybe I'm going in the right direction". In general I felt I was discovering the relevant places "on my own" and I didn't feel guided by metamath to go to the core files.

In any case my preference is not too strong, so I'm open to variations. I already appreciate the beginning of the transition from this PR.

@jkingdon
Copy link
Contributor

Therefore, merging this one will hasten those two ? ;)

We can always hope but there isn't really any guarantee that one would cause the other - a read through metamath/metamath-website-scripts#2 perhaps makes this clear.

@digama0
Copy link
Member

digama0 commented Sep 25, 2023

While I am of course in support of this general direction, as my stance on #1887 should make clear, the changes to HTML files in this PR worry me. Is this directory change being replicated on the metamath site? That is, will https://us.metamath.org/mpeuni/_relexample.svg be a 404 after this change? If yes, it might be a bit awkward that some but not all images are in the images directory. If no, the HTML files should not be modified (and we have to make a choice between making the HTML files work in situ in the github repo, or making them work on the website). And in either case, we need to modify the website script because it's not expecting the image files to be at this new location.

Put another way, we shouldn't merge this PR without a matching PR to metamath-website-scripts repo.

@jkingdon jkingdon dismissed their stale review September 25, 2023 15:04

This change will not work with current build scripts

@david-a-wheeler
Copy link
Member

I just got back from back-to-back work meetings (DC, Spain). I haven't had a chance to review this yet, but clearly we want to ensure that the results work :-).

Comment on lines +64 to -66
`scripts/rewrap set.mm` to make sure your pull request passes this check.

Locally you will need to run `scripts/rewrap set.mm` to avoid failing this
check.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"avoid failing" --> "make sure [it] passes": avoid double negatives

@benjub benjub requested a review from jkingdon October 29, 2023 15:31
Copy link
Contributor

@jkingdon jkingdon left a comment

Choose a reason for hiding this comment

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

I don't see any response to

Is this directory change being replicated on the metamath site? That is, will https://us.metamath.org/mpeuni/_relexample.svg be a 404 after this change? If yes, it might be a bit awkward that some but not all images are in the images directory. If no, the HTML files should not be modified (and we have to make a choice between making the HTML files work in situ in the github repo, or making them work on the website). And in either case, we need to modify the website script because it's not expecting the image files to be at this new location.

nor could I see based on looking through https://github.com/metamath/metamath-website-scripts/tree/main how the svg files get copied to the website (the test for whether it worked is whether they are visible in the table at https://us.metamath.org/mpeuni/mmfrege.html ).

The changes to CONTRIBUTING.md look good to me, perhaps that can productively be separated into a different pull request than the creation of an images directory.

@digama0
Copy link
Member

digama0 commented Oct 30, 2023

nor could I see based on looking through https://github.com/metamath/metamath-website-scripts/tree/main how the svg files get copied to the website (the test for whether it worked is whether they are visible in the table at https://us.metamath.org/mpeuni/mmfrege.html ).

I had the same question recently... the answer is that they aren't - the SVGs served on the website are actually https://github.com/metamath/metamath-website-seed/blob/main/mpegif/_frege_A.svg et al (and don't let the mpegif fool you - all the files in the seed/mpegif directory are copied to both mpegif and mpeuni, the only difference between these directories is for the autogenerated files). I think we should just remove them from this repo.

@benjub
Copy link
Contributor Author

benjub commented Oct 30, 2023

So, should I remove all these svg's in this repo ? But then, what URLs should I put as links ? Will the initial ones work unchanged ? If you confirm, then the easiest is that I close this MR and do that separately. (The part touching CONTRIBUTING.md is now a separate MR, #3603.)

@benjub benjub marked this pull request as draft October 30, 2023 22:28
@benjub
Copy link
Contributor Author

benjub commented Nov 12, 2023

So, should I remove all these svg's in this repo ? But then, what URLs should I put as links ? Will the initial ones work unchanged ? If you confirm, then the easiest is that I close this MR and do that separately. (The part touching CONTRIBUTING.md is now a separate MR, #3603.)

So: can I simply remove the svg's from this repo ? Should I update the links or will they work as is ?

@digama0
Copy link
Member

digama0 commented Nov 14, 2023

They should work as is, assuming they are working currently. Like I said, the current build process isn't touching the SVGs in this repo at all.

@benjub benjub mentioned this pull request Nov 18, 2023
@benjub
Copy link
Contributor Author

benjub commented Nov 18, 2023

Closing as subsumed by #3603 and #3639.

@benjub benjub closed this Nov 18, 2023
@benjub benjub deleted the images branch November 18, 2023 10:43
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.

7 participants