Skip to content

Conversation

@kwankyu
Copy link

@kwankyu kwankyu commented Dec 2, 2022

No description provided.

Copy link
Member

@soehms soehms left a comment

Choose a reason for hiding this comment

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

I think we should restrict the match to occurrences with a preceding whitespace to exclude cases of previously converted references (I mean matches of the form [display-text](https://trac.sagemath.org/ticket/([0-9]+)).

Another point that I've ignored by myself, so far: We should better use a global variable for https://trac.sagemath.org/ extracted from config data (trac_url).

@kwankyu
Copy link
Author

kwankyu commented Dec 3, 2022

I think we should restrict the match to occurrences with a preceding whitespace to exclude cases of previously converted references (I mean matches of the form [display-text](https://trac.sagemath.org/ticket/([0-9]+)).

Another point that I've ignored by myself, so far: We should better use a global variable for https://trac.sagemath.org/ extracted from config data (trac_url).

Done.

Copy link
Member

@soehms soehms left a comment

Choose a reason for hiding this comment

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

Sorry, it seems that the second part of my comment was misleading. More precisely, the meaning of the variable trac_ticket_url I introduced previously is confusing.

Unfortunately, I used the existence of trac_ticket_url implicitly to distinguish between wiki and ticket migration. A better name for it would have been keep_trac_ticket_references with boolean values. Thus, the position of your additional line of code has been better before, since we want it working on ticket migration, as well.

Now, my suggestion is the following.

  1. Replace ticket_url (defined in migrate.cfg.sagetracwikionly) by keep_trac_ticket_references: yes and import it as an optional boolean in migrate.py (default False).
  2. Define globally trac_url_dir = os.path.dirname(trac_url) and trac_ticket_url = os.path.join(trac_url_dir, 'ticket'). Then trac_ticket_url has the meaning that people expect.
  3. Move your additional line back to where it has been in your first commit. More precisely, preceding the line which now reads if keep_trac_ticket_references:. Furthermore, change r' [#\1](%s/\1)' % trac_ticket_url back to r' #\1.

After that also the migration of tickets would convert global ticket references to the corresponding GitHub issue reference.

Note that I have already rephrased the lines of code concerning trac_ticket_url (old meaning) in PR #19. I will adapt that according to your changes to avoid merge errors!

@kwankyu
Copy link
Author

kwankyu commented Dec 5, 2022

This is a confusion I worried about. I will simply withdraw this PR. Please replace it with your own PR.

On the other hand, I think that all links to trac tickets should be converted to real working links so that we have a fully working wiki before ticket migration happens.

@kwankyu kwankyu closed this Dec 5, 2022
@kwankyu kwankyu deleted the fix-item-A9 branch December 5, 2022 20:00
@soehms
Copy link
Member

soehms commented Dec 6, 2022

Sorry for the trouble!

@soehms
Copy link
Member

soehms commented Dec 6, 2022

Please replace it with your own PR.

This is part of #19, now.

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.

2 participants