Skip to content

Conversation

@st--
Copy link
Member

@st-- st-- commented Jun 30, 2021

correctly set name when run from same directory as script

correctly set name when run from same directory as script
@st-- st-- requested a review from devmotion June 30, 2021 14:29
@devmotion
Copy link
Member

This is not how literate.jl was intended to be used so I did not account for it. I think I added a comment regarding this behaviour in the examples-base PR and thus suggested that the documentation of how to run literate.jl there is fixed. If there is a need for this use case (I am not sure if this is the case), then maybe it is easier if you make changes in the other PR where you added this command of invoking literate.jl without a full path of script.jl since currently it should not be used intentionally and is not documented?

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

I may have misunderstood your comment then, I thought your suggestion was to use literate.jl instead of manually calling into Literate.markdown. (I'm not gonna touch #303 anymore, I just want it merged and out of the way 😆 it's already taken two weeks, and separating out the other examples from #234 will be easier with it being merged in. And in any case we can also amend the docs in this PR.)

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

If there is a need for this use case (I am not sure if this is the case),

seems to me to be the simplest way of locally building an example?

since currently it should not be used intentionally

Could you clarify why you think it shouldn't be used manually?

@devmotion
Copy link
Member

It just was not supposed to be used in this way, it was designed on purpose for cases where you provide a full path to script.jl. So the setup is designed for

julia path/to/literate.jl path/to/script.jl path/to/output

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

And why not merge this tiny change which would allow it to be just as easily called without full path?

@devmotion
Copy link
Member

devmotion commented Jun 30, 2021

I am completely fine with this change but I am confused by the fact that it would be merged before and independently of #303. As I see it, currently there is nothing to fix since literate.jl is not documented and one is supposed to provide the full path (this was a design choice). Only with the documentation and different use case in #303 this is something that could possibly be fixed.

@devmotion
Copy link
Member

Maybe just merge this into #303?

@devmotion
Copy link
Member

(You can change the base branch in the Github interface above)

@st--
Copy link
Member Author

st-- commented Jun 30, 2021

So it seems we can

  1. change base and merge this PR into Examples in the docs: base PR for cleaning up notebook workflow #303, then re-approve Examples in the docs: base PR for cleaning up notebook workflow #303
  2. approve this PR now and merge it into master before Examples in the docs: base PR for cleaning up notebook workflow #303 is merged
  3. approve this PR now and merge it into master after Examples in the docs: base PR for cleaning up notebook workflow #303 is merged
  4. approve this PR after Examples in the docs: base PR for cleaning up notebook workflow #303 is merged

The end result is the same. If it makes a big difference to you what the intermediate statuses of master are, I'm happy to wait for 4. I care more about how long it takes to get #303 merged than how long it takes to get this merged, so I was reluctant to go for option 1 which would dismiss the approval I finally got on #303. 😅

@devmotion
Copy link
Member

There were just some discussions and improvements needed in #303 so it took some time until it was approved 🤷 I don't think it's bad if one has to iterate and wait until PRs are approved, in fact, I think usually one has to fix and discuss different things 🙂

If you don't want to touch #303 anymore, I'd suggest merging #303 and updating this PR afterwards. Even though my personal feeling is that this belongs into #303 and would not add any additional delay.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass 🙂

@st-- st-- merged commit b150888 into master Jul 1, 2021
@st-- st-- deleted the st---patch-1 branch July 1, 2021 05:53
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.

3 participants