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

Some suggestions, this could be improved still. #1

Merged
merged 24 commits into from
Oct 4, 2024

Conversation

arran4
Copy link

@arran4 arran4 commented Jul 25, 2023

Hey, this is a pull request INTO your pull request.

There still might be some good work which could be done here.

It also occurred to me that if you were to put a "read counter" in:
https://github.com/arran4/golang-ical/blob/84e95bcb9f6760d1b288e655977a3fc5e2ae6fa9/calendar.go#L523-L533

golang-ical/calendar.go

Lines 523 to 533 in 84e95bc

type CalendarStream struct {
r io.Reader
b *bufio.Reader
}
func NewCalendarStream(r io.Reader) *CalendarStream {
return &CalendarStream{
r: r,
b: bufio.NewReader(r),
}
}

And put it in the function here;
https://github.com/arran4/golang-ical/blob/84e95bcb9f6760d1b288e655977a3fc5e2ae6fa9/calendar.go#L535-L590

golang-ical/calendar.go

Lines 535 to 590 in 84e95bc

func (cs *CalendarStream) ReadLine() (*ContentLine, error) {
r := []byte{}
c := true
var err error
for c {
var b []byte
b, err = cs.b.ReadBytes('\n')
if len(b) == 0 {
if err == nil {
continue
} else {
c = false
}
} else if b[len(b)-1] == '\n' {
o := 1
if len(b) > 1 && b[len(b)-2] == '\r' {
o = 2
}
p, err := cs.b.Peek(1)
r = append(r, b[:len(b)-o]...)
if err == io.EOF {
c = false
}
if len(p) == 0 {
c = false
} else if p[0] == ' ' || p[0] == '\t' {
_, _ = cs.b.Discard(1) // nolint:errcheck
} else {
c = false
}
} else {
r = append(r, b...)
}
switch err {
case nil:
if len(r) == 0 {
c = true
}
case io.EOF:
c = false
default:
if err != nil {
err = fmt.Errorf("readline: %w", err)
}
return nil, err
}
}
if len(r) == 0 && err != nil {
return nil, fmt.Errorf("readline: %w", err)
}
cl := ContentLine(r)
if err != nil {
err = fmt.Errorf("readline: %w", err)
}
return &cl, err
}

You could a line offset and with some smart refactoring a character offset inserted in the errors raised in:
https://github.com/arran4/golang-ical/blob/84e95bcb9f6760d1b288e655977a3fc5e2ae6fa9/calendar.go#L438-L521

golang-ical/calendar.go

Lines 438 to 521 in 84e95bc

func ParseCalendar(r io.Reader) (*Calendar, error) {
state := "begin"
c := &Calendar{}
cs := NewCalendarStream(r)
cont := true
for ln := 0; cont; ln++ {
l, err := cs.ReadLine()
if err != nil {
switch err {
case io.EOF:
cont = false
default:
return c, err
}
}
if l == nil || len(*l) == 0 {
continue
}
line, err := ParseProperty(*l)
if err != nil {
return nil, fmt.Errorf("%w %d: %w", ErrParsingLine, ln, err)
}
if line == nil {
return nil, fmt.Errorf("%w %d", ErrParsingCalendarLine, ln)
}
switch state {
case "begin":
switch line.IANAToken {
case "BEGIN":
switch line.Value {
case "VCALENDAR":
state = "properties"
default:
return nil, ErrMalformedCalendarExpectedVCalendar
}
default:
return nil, ErrMalformedCalendarExpectedBegin
}
case "properties":
switch line.IANAToken {
case "END":
switch line.Value {
case "VCALENDAR":
state = "end"
default:
return nil, ErrMalformedCalendarExpectedEnd
}
case "BEGIN":
state = "components"
default: // TODO put in all the supported types for type switching etc.
c.CalendarProperties = append(c.CalendarProperties, CalendarProperty{*line})
}
if state != "components" {
break
}
fallthrough
case "components":
switch line.IANAToken {
case "END":
switch line.Value {
case "VCALENDAR":
state = "end"
default:
return nil, fmt.Errorf("%w at '%s': %w", ErrMalformedCalendarExpectedEnd, line.IANAToken, err)
}
case "BEGIN":
co, err := GeneralParseComponent(cs, line)
if err != nil {
return nil, fmt.Errorf("%w at '%s': %w", ErrMalformedCalendarExpectedEnd, line.IANAToken, err)
}
if co != nil {
c.Components = append(c.Components, co)
}
default:
return nil, fmt.Errorf("%w at '%s'", ErrMalformedCalendarExpectedBeginOrEnd, line.IANAToken)
}
case "end":
return nil, ErrMalformedCalendarUnexpectedEnd
default:
return nil, ErrMalformedCalendarBadState
}
}
return c, nil
}

A bit like any other parser.

Anyway. Please review this, then make some improvements if you can find any and then we can merge upstream into master once done.

brackendawson and others added 13 commits September 25, 2024 02:08
And test the target forever
Should be able to distinguish unset from invalid time properties
Add method to remove property
* master: (54 commits)
  Add method to remove property
  Move targeted Go version to 1.20
  Remove deprecated ioutil
  Improve error for property not found
  Should be able to distinguish unset from invalid time properties
  Add ComponentPropertyRelatedTo; the VTODO component can have that
  fix: reduce build restriction on serialization test
  fix: add test err assertion
  refactor: switch syntax
  refactor: reduce scope of gitignore
  refactor: fix linting errors
  breaking: unescape Property.Value of type TEXT
  fix: VFREEBUSY serialization
  add: serialiation test
  add: stable serialization of property parameters
  fix: use macos-13, arm is not supported by setup-go
  fix: explicitly list golang versions
  fix: simplify & update ghas
  add: contributing.md
  Re-add Calendar.RemoveEvent method
  ...

# Conflicts:
#	components.go
Renamed interface as it's useful own it's own. Made it public. Added …
* master: (56 commits)
  Merged
  Add method to remove property
  Move targeted Go version to 1.20
  Remove deprecated ioutil
  Improve error for property not found
  Should be able to distinguish unset from invalid time properties
  Add ComponentPropertyRelatedTo; the VTODO component can have that
  fix: reduce build restriction on serialization test
  fix: add test err assertion
  refactor: switch syntax
  refactor: reduce scope of gitignore
  refactor: fix linting errors
  breaking: unescape Property.Value of type TEXT
  fix: VFREEBUSY serialization
  add: serialiation test
  add: stable serialization of property parameters
  fix: use macos-13, arm is not supported by setup-go
  fix: explicitly list golang versions
  fix: simplify & update ghas
  add: contributing.md
  ...
Add leaks and vunerability checks
* manolotonto1/master: (46 commits)
  fixed som functions that are using the wrong Struct Embeddings
  fixed some namings
  fixed merge conflicts
  Add ComponentPropertyRelatedTo; the VTODO component can have that
  fix: reduce build restriction on serialization test
  fix: add test err assertion
  refactor: switch syntax
  refactor: reduce scope of gitignore
  refactor: fix linting errors
  breaking: unescape Property.Value of type TEXT
  fix: VFREEBUSY serialization
  add: serialiation test
  add: stable serialization of property parameters
  fix: use macos-13, arm is not supported by setup-go
  fix: explicitly list golang versions
  fix: simplify & update ghas
  add: contributing.md
  Re-add Calendar.RemoveEvent method
  Only conditionally add in the mailto: prefix for attendee and organizer
  Prefix mailto: for email in organizer property
  ...

# Conflicts:
#	.github/workflows/test.yml
#	calendar_test.go
@arran4
Copy link
Author

arran4 commented Sep 29, 2024

@ManoloTonto1 This is my "reverse" pull request, a PR into your PR to fix some changes. I haven't reviewed if they are still relevant just fixed the conflicts so you can. This PR goes into arran4#73 which has stagnated.

I have replaced a couple of PRs with my own which tries to resolve issues with them and I will wait a short while before merging them myself (feel free to review them if you like.) I can take over yours ( arran4#73 ) too and replace it with my own (I will be keeping your commits so in essence I will be duplicating this pr #1 into mine. )

* master:
  New tool
  Merged
  Add method to remove property
  Move targeted Go version to 1.20
  Remove deprecated ioutil
  Improve error for property not found
  Should be able to distinguish unset from invalid time properties
  Renamed interface as it's useful own it's own. Made it public. Added a comment
  Requires secrets now
  Add leaks and vunerability checks

# Conflicts:
#	.github/workflows/test.yml
#	components.go
#	errors.go
#	go.mod
@arran4
Copy link
Author

arran4 commented Sep 29, 2024

Actually rebased it with master too. This should improve things. You will have to enable actions on your fork to see if the tests works before accepting the merge into your pull request sorry these things tend to be a bit confusing. I wish github supported "suggest a change" like gitlab has

@arran4
Copy link
Author

arran4 commented Sep 29, 2024

@ManoloTonto1 Fixed the failing test b/c of not picking up on the error wrapping of EOF. There are potentially some other of these issues around too.

@arran4
Copy link
Author

arran4 commented Sep 29, 2024

Did a quick spot check. But there could be more issues here too. I'm not 100% on the format strings, but they will work.

@ManoloTonto1
Copy link
Owner

ManoloTonto1 commented Oct 3, 2024

@arran4 shall we merge it?
Did a quick spot check also, looks good. Thanks for the patience, I haven't had enough time to sit down and fix more things.

@arran4
Copy link
Author

arran4 commented Oct 4, 2024

THis is a PR into your PR, so it's up to you. I suggest yes. But I can go around you and just merge it my branch into the "origin" repo.

I recommend that you merge it into yours for process sake. So feel free to request changes into your pr. :P

@arran4
Copy link
Author

arran4 commented Oct 4, 2024

But thanks. Happy to wait for you.

@ManoloTonto1 ManoloTonto1 merged commit 1d9da2e into ManoloTonto1:master Oct 4, 2024
@ManoloTonto1
Copy link
Owner

@arran4 Merged 🚀

@arran4 arran4 deleted the err_change branch October 6, 2024 01:12
@arran4
Copy link
Author

arran4 commented Oct 6, 2024

Alright, looks like some new conflicts have occurred. I will see where this leaves us.

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.

4 participants