search

wf49670 / ppgen

Post-processing generator for DP
6 stars 4 forks source link

warning if no units specified on illustrations #46

Closed ghost closed 9 years ago

ghost commented 9 years ago

Users are supposed to specify either "50%" or "300px" but there is an expectation that "300" would work, probably because "width='300'" defaulted to px even though "width:300" does not. Users have requested a warning be issued if any illustration is specified in source without either "%" or "px" units.

If someone can pick this up, that would be good. I'm floating face down in PHP right now. Otherwise I'll get to it eventually.

wf49670 commented 9 years ago

On 10/24/2014 12:11 PM, rfrank wrote:

Users are supposed to specify either "50%" or "300px" but there is an expectation that "300" would work, probably because "width='300'" defaulted to px even though "width:300" does not. Users have requested a warning be issued if any illustration is specified in source without either "%" or "px" units.

If someone can pick this up, that would be good. I'm floating face down in PHP right now. Otherwise I'll get to it eventually.

I'll do it, Roger.

To be sure: if neither px nor %, then use as specified (which we know will not work) and issue a warning.

Right?

Walt

ghost commented 9 years ago

Yes that would be good. My preference would be fatal not warn but let's go with the community choice.

Sent from my iPhone

On Oct 24, 2014, at 2:47 PM, wf49670 notifications@github.com wrote:

On 10/24/2014 12:11 PM, rfrank wrote:

Users are supposed to specify either "50%" or "300px" but there is an expectation that "300" would work, probably because "width='300'" defaulted to px even though "width:300" does not. Users have requested a warning be issued if any illustration is specified in source without either "%" or "px" units.

If someone can pick this up, that would be good. I'm floating face down in PHP right now. Otherwise I'll get to it eventually.

I'll do it, Roger.

To be sure: if neither px nor %, then use as specified (which we know will not work) and issue a warning.

Right?

Walt — Reply to this email directly or view it on GitHub.

wf49670 commented 9 years ago

On 10/24/2014 4:59 PM, rfrank wrote:

Yes that would be good. My preference would be fatal not warn but let's go with the community choice.

I agree that a fatal error would be better, given that other errors in .il are fatal, and that we know the illo will not work correctly. Perhaps the community did not understand that?

Anyway, warning it shall be, but a strongly worded one :)

(Or, possibly, the community thought you meant that ppgen would treat it as though px were specified, thus making it work, but issue a warning?)

Walt

wf49670 commented 9 years ago

On 10/24/2014 4:59 PM, rfrank wrote:

Yes that would be good. My preference would be fatal not warn but let's go with the community choice.

Of course, the clear advantage of warning over fatal is that you can gather a list of all the things you need to change with one run of the program, and then fix them all at once, rather than finding an error, fixing it, rerunning, finding the next error, etc.

So where there is sufficient information to generate something, then perhaps warning is better for the PPer.

Walt

wf49670 commented 9 years ago

On 10/24/2014 4:59 PM, rfrank wrote:

Yes that would be good. My preference would be fatal not warn but let's go with the community choice.

Here's the message I have at the moment: self.warn("image width (w=) does not end in px or %. The image will not display properly:\n {}").format(s)

Suggestions for something different?

Walt

ghost commented 9 years ago

Again I don't understand. I did the merge that was requested and now it says develop is 8 commits ahead of master. That, and this issue is still open. What did I just close?

wf49670 commented 9 years ago

On 10/24/2014 9:21 PM, rfrank wrote:

Again I don't understand. I did the merge that was requested and now it says develop is 8 commits ahead of master. That, and this issue is still open. What did I just close?

You closed my pull request.

It was #47, and the issue you opened was #46.

Walt

ghost commented 9 years ago

Ok so I can close this now as there is no commit coming.

Develop is way ahead of master is still a mystery. I thought I just made them even this morning.

wf49670 commented 9 years ago

On 10/24/2014 11:33 PM, rfrank wrote:

Ok so I can close this now as there is no commit coming.

Develop is way ahead of master is still a mystery. I thought I just made them even this morning.

I cannot explain why develop would remain ahead of master. I ran some experiments on a test repository that I setup, and cannot duplicate that.

My experiment: (1) I made a change to branch develop on my PC, then pushed it to remote/origin develop.

(2) Looking at develop on github, it is then 1 commit ahead of master.

(3) I made another change to branch develop on my PC, and pushed it to remote/origin develop.

(3) On github develop is then 2 commits ahead of master.

(4) On github I did a pull from develop into master.

(5) On github develop is then 1 commit behind master.

Repeating the above a few times, after the pull develop is increasingly behind master by 2, 3, ... commits. After the merge (pull from develop into master) develop always remains behind master, until I push something new into develop. At that point it is 1 ahead of, and 1 (or more) behind master.

Note that I did all the merges (pulls) on the github website. Using that method, the "pull" process consists of (a) create the pull request (b) confirm the pull request

After (b) the website prompts me to delete branch develop. If I do that, and then immediately use the website to create branch develop again, it is even with master.

Alternative workflow: Since you're working with collaborators, this might work for you. Suppose I create a pull request so you can pull changes from "wf49670/ppgen develop" (or any other branch I have) into "rfrank/ppgen develop". Your steps would be:

(a) confirm the pull request on the github website.

(b) then, on your PC, (b1) checkout develop (b2) remote pull from origin (b3) local merge into develop from origin develop.

(c) do whatever testing and examination of the source you wish.

(d) on the PC: (d1) checkout master (d2) local merge develop into master (d3) remote push develop to origin develop (d4) remote push master to origin master

At that point, both develop and master should have equal contents, and be even on the github website. Step (d3) is the key to making that happen, I think. (But I cannot easily test that with my current configuration.)

Walt