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

disable respace #879

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

disable respace #879

wants to merge 4 commits into from

Conversation

burrbull
Copy link
Member

@burrbull burrbull commented Oct 25, 2024

Closes #858

@burrbull
Copy link
Member Author

/ci diff pr

Copy link

Diff for comment

@burrbull
Copy link
Member Author

/ci diff pr

Copy link

Diff for comment

@burrbull burrbull force-pushed the respace branch 2 times, most recently from 997e12c to 6b84925 Compare March 9, 2025 05:32
@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

/ci diff pr

Copy link

github-actions bot commented Mar 9, 2025

Diff for comment

@burrbull burrbull changed the title [test] disable respace disable respace Mar 9, 2025
@burrbull burrbull marked this pull request as ready for review March 9, 2025 06:02
@burrbull burrbull requested a review from a team as a code owner March 9, 2025 06:02
@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

@Emilgardis Let's merge this too. This should be done in svdtools, not in svd2rust.

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

cc @thejpster

@burrbull burrbull marked this pull request as draft March 9, 2025 09:14
@Emilgardis
Copy link
Member

why was this made into a draft again?

@burrbull burrbull marked this pull request as ready for review March 9, 2025 09:25
@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

why was this made into a draft again?

I took some time to rewrite respace with Cow and fix one issue. See last commit.

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

/ci diff pr

Copy link

github-actions bot commented Mar 9, 2025

Diff for comment

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

/ci diff -c rp2350 pr

Copy link

github-actions bot commented Mar 9, 2025

Diff for comment

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

/ci diff pr

Copy link

github-actions bot commented Mar 9, 2025

Diff for comment

@Emilgardis
Copy link
Member

I dont enjoy the output :/ doesn't look good

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

I dont enjoy the output :/ doesn't look good

What do you suggest?

@Emilgardis
Copy link
Member

Hmm, I guess the problem I have is how the source material is formatted. is there a issue/pr for improving the descriptions in the patched svds?

@burrbull
Copy link
Member Author

burrbull commented Mar 9, 2025

is there a issue/pr for improving the descriptions in the patched svds?

For adequate support svdtools requires regex replace.

But I still not sure about syntax:

  1. description regex replace svdtools#252: easiest, but support descriptions only
  2. rust-embedded/svdtools@master...regex-replace more universal, but more complex code and patch rules
  3. allow for each string entry of svd_rs::XxBuilder to take "replace string". Something similar to sed maybe.

4.) always process this special case: replace \n\s+ with .

@Emilgardis
Copy link
Member

Emilgardis commented Mar 10, 2025

Could we reverse the information about where the text in the description starts, then apply that spacing and then do a "unindent"?

@burrbull
Copy link
Member Author

Could we reverse the information about where the text in the description starts, then apply that spacing and then do a "unindent"?

Could you show example? I don't understand what you mean.

@Emilgardis
Copy link
Member

take for example this:

# line 1542 in stm32f303.svd.patched
      <interrupt>
        <name>PVD</name>
        <description>PVD through EXTI line detection
        interrupt</description>
        <value>1</value>
      </interrupt>

this gets translated to

#[doc = "1 - PVD through EXTI line detection\n        interrupt"] 

instead, I think since we can, via text_pos_at get how much of that is indentation to make the xml look nice and how much is indentation for formatting, we then use the same logic unindent uses to make the doc string

#[doc = "1 - PVD through EXTI line detection\ninterrupt"]

however, looking at the doc for text_pos_at they say it's expensive, so maybe not worth it.

@burrbull
Copy link
Member Author

instead, I think since we can, via text_pos_at get how much of that is indentation to make the xml look nice and how much is indentation for formatting, we then use the same logic unindent uses to make the doc string

You mean to check that text indentation is equal to tag indentation?

@Emilgardis
Copy link
Member

Yes, essentially.

I'm fine with merging this however, but I'm a bit suspicious about if we're really handling this in a good way or not.

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.

Whitespace in SVD files is lost
2 participants